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

Merged
merged 10 commits into from Aug 17, 2016

Conversation

4 participants
@zcorpan
Member

zcorpan commented Aug 16, 2016

From https://rocallahan.github.io/innerText-spec/ with no normative
changes except adding [CEReactions] to the IDL.

Fixes #465.


@Ms2ger @rocallahan

Upstream the innerText spec
From https://rocallahan.github.io/innerText-spec/ with no normative
changes except adding [CEReactions] to the IDL.

Fixes #465.

zcorpan added some commits Aug 17, 2016

source Outdated
<code>select</code>, and <code>video</code> &mdash; 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

This comment has been minimized.

@annevk

annevk Aug 17, 2016

Member

"a ranges" -> "a range" | "ranges"

source Outdated
<li><p>Return the concatenation of the string items.</p></li>
</ol>
<p class="note">Note that descendant nodes of most replaced elements (e.g. <code>textarea</code>,

This comment has been minimized.

@annevk

annevk Aug 17, 2016

Member

comma after e.g.

zcorpan added some commits Aug 17, 2016

Add [TreatNullAs=EmptyString] to innerText
This matches WebKit/Chromium but not Gecko/IE. Treating null as
empty string is consistent with textContent and innerHTML.
source Outdated
these steps:</p>
<ol>
<li><p>If the element is not <span>being rendered</span>, return the same value as the

This comment has been minimized.

@annevk

annevk Aug 17, 2016

Member

@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.

source Outdated
<li>
<p>Compute a list of items each of which is a string or a "required line break count", a
positive integer, by applying the following recursive procedure to each child node
<var>node</var> of the element in <span>tree order</span> and concatenating the results.

This comment has been minimized.

@annevk

annevk Aug 17, 2016

Member

Make this end in parenthesis?

This comment has been minimized.

@zcorpan

zcorpan Aug 17, 2016

Member

Hmm I suppose " and concatenating the results" should be removed here.

This comment has been minimized.

@zcorpan

zcorpan Aug 17, 2016

Member

Hmm, no this should still be there. Each child produces a list, and we want to concatenate those lists into one...

source Outdated
<var>node</var> of the element in <span>tree order</span> and concatenating the results.
(Intuitively, a "required line break count" item means that a certain number of line breaks must
appear at that point, but they can be collapsed with the line breaks induced by adjacent
"required line break count" items, reminiscient to CSS margin-collapsing.)</p>

This comment has been minimized.

@annevk

annevk Aug 17, 2016

Member

This looks like a note, mark it up as such and avoid normative requirements?

source Outdated
of <var>node</var> and concatenating the results (producing a list of items).</p></li>
<li><p>If <var>node</var>'s <span>computed value</span> of <span>'visibility'</span> is not
'visible', return <var>items</var>.</p></li>

This comment has been minimized.

@annevk

annevk Aug 17, 2016

Member

then return*

source Outdated
'visible', return <var>items</var>.</p></li>
<li>
<p>If <var>node</var> is a <code>Text</code> node child of an <code>rp</code> element, return

This comment has been minimized.

@annevk

annevk Aug 17, 2016

Member

then return*

This comment has been minimized.

@annevk

annevk Aug 17, 2016

Member

This happens a few more times.

source Outdated
only removed if the line is the last line of the block, or it ends with a <code>br</code>
element. Soft hyphens should be preserved. <ref spec=CSSTEXT></p></li>
<li><p>If <var>node</var> is a <code>br</code> element, append a string containing a single

This comment has been minimized.

@annevk

annevk Aug 17, 2016

Member

then append

source Outdated
<li><p>Delete any string items whose strings are empty.</p></li>
<li><p>Delete any runs of consecutive "required line break count" items at the start or end of
the list.</p></li>

This comment has been minimized.

@annevk

annevk Aug 17, 2016

Member

This should presumably do some starts with matching? We only add "required line break count of ..." after all.

This comment has been minimized.

@zcorpan

zcorpan Aug 17, 2016

Member

I don't follow.

This comment has been minimized.

@annevk

annevk Aug 17, 2016

Member

The string being added is "required line break count of 2" / "required line break count of 1", not this string.

This comment has been minimized.

@zcorpan

zcorpan Aug 17, 2016

Member

Ah, I see. Those items are not strings. They are integers, basically. I suppose it could be made less confusing...

source Outdated
<p>Returns the element's text content "as rendered".</p>
<p>Can be set, to replace the element's children with the given value, but with linebreaks

This comment has been minimized.

@annevk

annevk Aug 17, 2016

Member

line breaks

source Outdated
steps:</p>
<ol>
<li><p>Let <var>document</var> be the given element's <span>node document</span>.</p></li>

This comment has been minimized.

@annevk

annevk Aug 17, 2016

Member

this element's

source Outdated
<li><p>Let <var>fragment</var> be a new <code>DocumentFragment</code> object whose <span>node
document</span> is <var>document</var>.</p></li>
<li><p>Let <var>input</var> be the new value being assigned.</p></li>

This comment has been minimized.

@annevk

annevk Aug 17, 2016

Member

"the new value being assigned" -> "the given value"

zcorpan added some commits Aug 17, 2016

@annevk

This comment has been minimized.

Member

annevk commented Aug 17, 2016

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.

@zcorpan

This comment has been minimized.

Member

zcorpan commented Aug 17, 2016

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>

This comment has been minimized.

@domenic

domenic Aug 17, 2016

Member

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.

This comment has been minimized.

@zcorpan

zcorpan Aug 17, 2016

Member

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.

<p>Can be set, to replace the element's children with the given value, but with line breaks
converted to <code>br</code> elements.</p>
</dl>

This comment has been minimized.

@domenic

domenic Aug 17, 2016

Member

Should we add a warning about how developers usually want to use textContent instead?

This comment has been minimized.

@zcorpan

zcorpan Aug 17, 2016

Member

I don't know, it's not clear to me that is usually true. For getting, innerText seems a lot more useful if you want the text but not the contents of scripts and don't want formatting of the HTML source to affect the given text. For setting, you would have to use <pre> but you might want spaces to collapse or don't want monospace font, or you could use <p> with white-space: pre-line (or so) but then you rely on CSS to preserve presumably important line breaks.

textContent is gives more consistent results cross-browser, though.

source Outdated
<p class="note">Intuitively, a <i>required line break count</i> item means that a certain number
of line breaks appear at that point, but they can be collapsed with the line breaks induced by
adjacent integer items, reminiscient to CSS margin-collapsing.</p>

This comment has been minimized.

@domenic

domenic Aug 17, 2016

Member

s/integer items/required line break count items?

Spelling is "reminiscent"

source Outdated
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>

This comment has been minimized.

@domenic

domenic Aug 17, 2016

Member

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...

This comment has been minimized.

@zcorpan

zcorpan Aug 17, 2016

Member

Yeah... I went with your suggestion for now but it's not great.

source Outdated
<li>
<p>If <var>node</var> is a <code>Text</code> node child of an <code>rp</code> element, then
return a single string item containing the text of <var>node</var>.

This comment has been minimized.

@domenic
source Outdated
<li><p>If <var>node</var>'s <span>computed value</span> of <span>'display'</span> is
<span>'table-cell'</span>, and <var>node</var>'s CSS box is not the last
<span>'table-cell'</span> box of its enclosing 'table-row' box, then append a string containing

This comment has been minimized.

@domenic

domenic Aug 17, 2016

Member

<span> around 'table-row'?

source Outdated
<li><p><span data-x="concept-node-append">Append</span> the result of <span
data-x="create an element">creating an element</span> given <var>document</var>,
<code>br</code>, and the <span>HTML namespace</span>, to <var>fragment</var>.</p></li>

This comment has been minimized.

@domenic

domenic Aug 17, 2016

Member

No comma after HTML namespace, I think

@domenic

This comment has been minimized.

Member

domenic commented Aug 17, 2016

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

@zcorpan zcorpan merged commit 132a55e into master Aug 17, 2016

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details

@zcorpan zcorpan deleted the innerText branch Aug 17, 2016

@smaug----

This comment has been minimized.

Collaborator

smaug---- commented Aug 18, 2016

I assume there will be tests for this in wpt.

zcorpan added a commit to web-platform-tests/wpt that referenced this pull request Aug 18, 2016

Ms2ger added a commit to web-platform-tests/wpt that referenced this pull request Aug 18, 2016

@zcorpan

This comment has been minimized.

Member

zcorpan commented Aug 19, 2016

@smaug---- there were tests in wpt already, and I have submitted these PRs to update them so far:
web-platform-tests/wpt#3482 (merged)
web-platform-tests/wpt#3483 (merged)
web-platform-tests/wpt#3491
web-platform-tests/wpt#3492
web-platform-tests/wpt#3493

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment