1484855 - InnerText doesn't use the spec definition for "being rendered"

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect, P2)

Description

[Emilio Cobos Álvarez (:emilio)]

Attachment: test.html

The spec says:

> An element is being rendered if it has any associated CSS layout boxes, SVG layout boxes, or some equivalent in other styling languages.

Blink follows that almost to the letter for innerText:

  https://cs.chromium.org/chromium/src/third_party/blink/renderer/core/dom/element.cc?type=cs&q=::innerText&sq=package:chromium&g=0&l=3774

Modulo display: contents. I filed https://github.com/whatwg/html/issues/3947 for that.

Gecko implements a weird IsOrHasAncestorWithDisplayNone thing, which is not equivalent for fallback content.

This can be seen in the attachment. I think all the cases should return the same (textContent, that is, "abc").

And thus that GetInnerText should just do something like:

if (!GetPrimaryFrame() && !IsDisplayContents()) {
  return GetTextContent(..);
}

Comment 1

[Emilio Cobos Álvarez (:emilio)]

Err.

Comment 2

[ExE Boss]

Attachment: dom-element-innertext-test.html

This my improved version of the test which doesn’t use `document.write(…)` (🤮) and instead uses `document.createTextNode(…)`, `element.appendChild(…)` and template literals.

This also uses element ids, rather than CSS selectors to refer to the test elements, so that the result looks better:
```
"#canvas-fallback" innerText: "", textContent: "abc"
"#video-source" innerText: "abc", textContent: "abc"
"#audio-source" innerText: "abc", textContent: "abc"
"#display-none" innerText: "abc", textContent: "abc"
```
(results from latest nightly)

Comment 3

[Emilio Cobos Álvarez (:emilio)]

Attachment: Bug 1484855: Match the 'is rendered' definition from the spec in innerText. r=smaug

This matches other implementations and the spec for fallback content like:

  <canvas><div>abc

(calling div.innerText).

We're treating the <div> as 'rendered' because it's not in a display: none
subtree, but that's not ok, since it is in fact not rendered.

This was added in bug 1226293, and Boris suggested this change, but roc opposed
because it'd be hard to spec properly in comment 15. Looks like the HTML spec
ended up merging roc's innerText spec, and now it's spec'd in terms of 'being
rendered'.

I think IsOrHasAncestorWithDisplayNone just doesn't work in any reasonable way
for stuff out of the flat tree. Thus I think this change is the right thing.

Comment 4

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 9002759 [details]
Bug 1484855: Match the 'is rendered' definition from the spec in innerText. r=smaug

Olli Pettay [:smaug] has approved the revision.

Comment 5

[Emilio Cobos Álvarez (:emilio)]

Olli, since I can't re-request review in Phab, could you take a look at the test changes from:

  https://phabricator.services.mozilla.com/D3887?vs=9556&id=9624

The canvas thing is bug 1485076 (not that I changed the test, but we preserve the behavior, wrongly). The rest match other engines.

Comment 6

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 9002759 [details]
Bug 1484855: Match the 'is rendered' definition from the spec in innerText. r=smaug

Err, see comment 5.

Comment 7

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 9002759 [details]
Bug 1484855: Match the 'is rendered' definition from the spec in innerText. r=smaug

Olli Pettay [:smaug] has approved the revision.

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/db66903f54cc
Match the 'is rendered' definition from the spec in innerText. r=smaug

Comment 9

[Web Platform Test Sync Bot [:wpt-sync] (Matrix: #interop:mozilla.org)]

Created web-platform-tests PR https://github.com/web-platform-tests/wpt/pull/12611 for changes under testing/web-platform/tests

Comment 10

[Web Platform Test Sync Bot [:wpt-sync] (Matrix: #interop:mozilla.org)]

Upstream web-platform-tests status checks passed, PR will merge once commit reaches central.

Comment 11

[Noemi Erli[:noemi_erli]]

https://hg.mozilla.org/mozilla-central/rev/db66903f54cc

Comment 12

[Web Platform Test Sync Bot [:wpt-sync] (Matrix: #interop:mozilla.org)]

Upstream PR merged