Closed Bug 1484855 Opened 2 years ago Closed 2 years ago

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

Categories

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

defect

Tracking

()

RESOLVED FIXED
mozilla63
Tracking Status
firefox63 --- fixed

People

(Reporter: emilio, Assigned: emilio)

References

(Blocks 1 open bug)

Details

Attachments

(2 files, 1 obsolete file)

Attached file test.html (obsolete) —
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(..);
}
Flags: needinfo?(emilio)
Err.
Flags: needinfo?(emilio)
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)
Attachment #9002612 - Attachment is obsolete: true
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.
Assignee: nobody → emilio
Flags: needinfo?(emilio)
Blocks: html
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.
Attachment #9002759 - Flags: review+
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 on attachment 9002759 [details]
Bug 1484855: Match the 'is rendered' definition from the spec in innerText. r=smaug

Err, see comment 5.
Attachment #9002759 - Flags: review+ → review?(bugs)
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.
Attachment #9002759 - Flags: review+
Attachment #9002759 - Flags: review?(bugs)
Priority: -- → P2
Attachment #9002759 - Attachment description: Bug 1484855: Match the 'is rendered' definition from the spec. r=smaug → Bug 1484855: Match the 'is rendered' definition from the spec in innerText. r=smaug
Pushed by emilio@crisal.io:
https://hg.mozilla.org/integration/autoland/rev/db66903f54cc
Match the 'is rendered' definition from the spec in innerText. r=smaug
Created web-platform-tests PR https://github.com/web-platform-tests/wpt/pull/12611 for changes under testing/web-platform/tests
Upstream web-platform-tests status checks passed, PR will merge once commit reaches central.
https://hg.mozilla.org/mozilla-central/rev/db66903f54cc
Status: NEW → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla63
You need to log in before you can comment on or make changes to this bug.