Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Specify outerText #6653

Merged
merged 5 commits into from
May 7, 2021
Merged

Specify outerText #6653

merged 5 commits into from
May 7, 2021

Conversation

domenic
Copy link
Member

@domenic domenic commented May 5, 2021 •
edited by pr-preview bot
Loading

Closes #668.

(See WHATWG Working Mode: Changes for more details.)

/custom-elements.html ( diff )
/dom.html ( diff )
/index.html ( diff )
/infrastructure.html ( diff )

domenic
domenic commented May 5, 2021
View reviewed changes
source

<li><p>Let <var>input</var> be the given value.</p></li>
<li><p>Let <var>fragment</var> be the <span>inner/outer text fragment</span> for the given value
given <span>this</span>'s <span>node document</span>.</p></li>
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Both Chromium and WebKit have a check after this point to see if the parent has disappeared due to mutation events. However I don't see how that'd be possible; no mutation events can fire due to the fragment creation, I am pretty sure. I'll see if any tests show that I'm wrong...

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I couldn't find any tests in Chromium's corpus that tried to trigger the corresponding if statement.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've confirmed that not only could I not find them, they do not exist, according to our code coverage metrics. I' m not sure how much time I really want to devote to this, but I think the right thing to do would be to omit such a check from the spec, and have Chromium convert the check into an assert or use counter and see if it triggers anywhere in the wild.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As discussed, I've added a NOTREACHED to that branch, and I'll wait a few milestones to see if it ever gets triggered (by fuzzers or tests or whatever).

chromium-wpt-export-bot pushed a commit to web-platform-tests/wpt that referenced this pull request May 5, 2021
@domenic @chromium-wpt-export-bot
Upstream and expand outerText tests
b274a5f 
This accompanies whatwg/html#6653.

Fixed: 710764
Change-Id: I7168df9bbaeef12ed256b7f2fb9c2c43c9d6b227
@chromium-wpt-export-bot chromium-wpt-export-bot mentioned this pull request May 5, 2021
Merged
@domenic domenic added addition/proposal New features or enhancements interop Implementations are not interoperable with each other labels May 5, 2021
annevk
annevk reviewed May 6, 2021
View reviewed changes
source
<h4 id="the-innertext-idl-attribute">The <code data-x="dom-innerText">innerText</code> getter and
setter</h4>
<h4 id="the-innertext-idl-attribute">The <code data-x="dom-innerText">innerText</code> and <code
data-x="dom-outerText">outerText</code> properties</h4>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not getters and setters?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It was getting too long so I thought this'd be nicer.

source Outdated Show resolved Hide resolved
source

<li><p>If <var>next</var> is non-null and <var>next</var>'s <span>previous sibling</span> is a
<code>Text</code> node, then <span>merge with the next text node</span> given <var>next</var>'s
<span>previous sibling</span>.</p></li>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The previous sibling here could basically be anything at this point, if we're accounting for mutation events. But I guess this is how it works?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, we bail out if previous sibling (or previous sibling's next sibling) is not a Text node.

source Show resolved Hide resolved
annevk
annevk approved these changes May 7, 2021
View reviewed changes
source
steps are to return the <span>rendered text</span> of <span>this</span>.</p>

<p>The <dfn attribute for="HTMLElement"><code data-x="dom-outerText">outerText</code></dfn> getter
steps are to return the <span>rendered text</span> of <span>this</span>.</p>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Optional: one thing we could do to make it even clearer they are identical is write this as

The innerText and outerText getter steps are to ...

@domenic domenic merged commit f05114b into main May 7, 2021
@domenic domenic deleted the outertext branch May 7, 2021 15:55
@domenic domenic added the impacts documentation Used by documentation communities, such as MDN, to track changes that impact documentation label May 7, 2021
@domenic
Copy link
Member Author

domenic commented May 7, 2021

/cc @whatwg/documentation

chromium-wpt-export-bot pushed a commit to web-platform-tests/wpt that referenced this pull request May 7, 2021
@domenic @chromium-wpt-export-bot
Upstream and expand outerText tests
b1c25ca 
This accompanies whatwg/html#6653.

Fixed: 710764
Change-Id: I7168df9bbaeef12ed256b7f2fb9c2c43c9d6b227
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2875725
Reviewed-by: Mason Freed <[email protected]>
Commit-Queue: Domenic Denicola <[email protected]>
Cr-Commit-Position: refs/heads/master@{#880460}
chromium-wpt-export-bot pushed a commit to web-platform-tests/wpt that referenced this pull request May 7, 2021
@domenic @chromium-wpt-export-bot
Upstream and expand outerText tests
e2bfe7b 
This accompanies whatwg/html#6653.

Fixed: 710764
Change-Id: I7168df9bbaeef12ed256b7f2fb9c2c43c9d6b227
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2875725
Reviewed-by: Mason Freed <[email protected]>
Commit-Queue: Domenic Denicola <[email protected]>
Cr-Commit-Position: refs/heads/master@{#880460}
pull bot pushed a commit to Mu-L/chromium that referenced this pull request May 8, 2021
@domenic
Upstream and expand outerText tests
c6af55f 
This accompanies whatwg/html#6653.

Fixed: 710764
Change-Id: I7168df9bbaeef12ed256b7f2fb9c2c43c9d6b227
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2875725
Reviewed-by: Mason Freed <[email protected]>
Commit-Queue: Domenic Denicola <[email protected]>
Cr-Commit-Position: refs/heads/master@{#880460}
moz-v2v-gh pushed a commit to mozilla/gecko-dev that referenced this pull request May 9, 2021
@domenic @moz-wptsync-bot
Bug 1709674 [wpt PR 28846] - Upstream and expand outerText tests, a=t…
5b08d9b 
…estonly

Automatic update from web-platform-tests
Upstream and expand outerText tests

This accompanies whatwg/html#6653.

Fixed: 710764
Change-Id: I7168df9bbaeef12ed256b7f2fb9c2c43c9d6b227
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2875725
Reviewed-by: Mason Freed <[email protected]>
Commit-Queue: Domenic Denicola <[email protected]>
Cr-Commit-Position: refs/heads/master@{#880460}

--

wpt-commits: e2bfe7b3d06bb26ff4d800087df6aad417feadfa
wpt-pr: 28846
@hamishwillee hamishwillee mentioned this pull request Feb 8, 2022
Merged
mjfroman pushed a commit to mjfroman/moz-libwebrtc-third-party that referenced this pull request Oct 14, 2022
@domenic @copybara-github
Upstream and expand outerText tests
db47ec1 
This accompanies whatwg/html#6653.

Fixed: 710764
Change-Id: I7168df9bbaeef12ed256b7f2fb9c2c43c9d6b227
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2875725
Reviewed-by: Mason Freed <[email protected]>
Commit-Queue: Domenic Denicola <[email protected]>
Cr-Commit-Position: refs/heads/master@{#880460}
NOKEYCHECK=True
GitOrigin-RevId: c6af55fb96b25ca3047ccc8675268e22959bd5f7
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mfreed7 mfreed7 mfreed7 left review comments

@annevk annevk annevk approved these changes

Assignees
No one assigned
Labels
addition/proposal New features or enhancements impacts documentation Used by documentation communities, such as MDN, to track changes that impact documentation interop Implementations are not interoperable with each other
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Spec HTMLElement.outerText
3 participants
@domenic @annevk @mfreed7