-
Notifications
You must be signed in to change notification settings - Fork 2.8k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Upstream the innerText spec #1678
Conversation
From https://rocallahan.github.io/innerText-spec/ with no normative changes except adding [CEReactions] to the IDL. Fixes #465.
<code>select</code>, and <code>video</code> — but not <code>button</code>) are not rendered | ||
by CSS, strictly speaking, and therefore have no CSS boxes for the purposes of this algorithm.</p> | ||
|
||
<p class="big-issue">This algorithm is amenable to being generalized to work on a <span |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"a ranges" -> "a range" | "ranges"
these steps:</p> | ||
|
||
<ol> | ||
<li><p>If the element is not <span>being rendered</span>, return the same value as the |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@domenic and I started using "this element" (or equivalent) in other algorithms. I think that would be a clearer way to refer to the object on which the IDL attribute is defined.
steps:</p> | ||
|
||
<ol> | ||
<li><p>Let <var>document</var> be the given element's <span>node document</span>.</p></li> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this element's
This looks okay. The only thing I'd consider changing further is making the recursion less declarative. Have that be some algorithm that is invoked for each child and also put the result in a variable of some kind the rest of the algorithm uses. Will let @domenic make the call on that. |
Yeah, it could use some more cleanup. Possibly also switch to iterative traversal instead of recursive? But I think the current state is OK to merge and I can fix more later. |
@@ -11962,6 +11974,175 @@ interface <dfn>DOMStringMap</dfn> { | |||
</div> | |||
|
|||
|
|||
<h4>The <code data-x="dom-innerText">innerText</code> IDL attribute</h4> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wonder if a better title might be something like Manipulating an element's "as rendered" text
? It seems to me like other section titles usually don't talk about a specific IDL attribute. I don't feel strongly though.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's a bit of a mix currently. Having "innerText" in the heading makes it easier to find. But if we also add outerText
we can maybe change the heading at that point.
list of items.</p></li> | ||
|
||
<li><p>If <var>node</var>'s <span>computed value</span> of <span>'visibility'</span> is not | ||
'visible', then return <var>items</var>.</p></li> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess this is the same concern as the factoring thing @annevk mentioned, but saying "return" inside a getter and that not being the result of the getter is pretty confusing. "Let the result of these substeps be items and abort these substeps"? Meh...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah... I went with your suggestion for now but it's not great.
LGTM. I think the original commit message is a bit outdated so I'll let you merge and write up a commit message. Also probably want to reference #1679 |
I assume there will be tests for this in wpt. |
@smaug---- there were tests in wpt already, and I have submitted these PRs to update them so far: |
From https://rocallahan.github.io/innerText-spec/ with no normative
changes except adding [CEReactions] to the IDL.
Fixes #465.
@Ms2ger @rocallahan