1567310 - Inspector shouldn't cause the creation of whitespace-only text frames.

Status: RESOLVED FIXED

(DevTools :: Inspector, enhancement, P2)

Description

[Emilio Cobos Álvarez (:emilio)]

There is a fair amount of layout bugs that we can't use devtools to investigate because opening inspector changes the rendering.

The reason for this, last I checked, is that inspector ended up calling getBoxQuads() and such on text nodes, which means that we need to create frames for those text-nodes, since we need to position them. Sometimes they mask the bug, see bug 1567098 for the latest example.

Would it be too hard to not do this?

Comment 1

[Razvan Caliman [:rcaliman]]

Sounds really inconvenient for platform developers to have DevTools change the layout for them while inspecting.

On a cursory look, I see a few places where getBoxQuads() is used in the DevTools in such as way as to measure the content on the inspected page:

• within a util on the server, nodeHasSize(), being called when filtering the DOM with standardTreeWalkerFilter() which is then used in the walker and document-walker. This ends up called when opening the Inspector as part of a node selection/traversal.
• within a util to get adjusted quads for highlighters (ex: element highlighter, shape path editor, grid/flex, etc) to account for iframe offsets and zoom levels. This seems important enough to keep.

Without digging too deep, it seems that the node filter used in DOM traversal is most likey to cause the issues because of its automatic use of getBoxQuads() in the process of node traversal. This is part of the code I have little experience with yet. I will ping Patrick and Gabriel for their assessment if we can circumvent or restrict the use of getBoxQuads() in that scenario.

Comment 2

[Patrick Brosset <:pbro>]

getBoxQuads, ever since it was introduced, has been a life changer for us on devtools because of the insights it gave us into the layout of a page. So, yeah, we tend to use it quite extensively in the inspector at least. I have to say that we always assumed it was a safe API for us to call, one that wouldn't change the thing being inspected. But it's true that through the years, there's been the occasional bugs of "opening devtools fixes it" and I don't think we've ever understood why, until now.

In particular here, I think Razvan's first find is the right one. We indeed use getBoxQuads on text nodes, and here is the reason:

Stuff like tabs and line breaks in HTML files are usually collapsed and most of the time, people don't really understand the way they do. On top of this, DOM inspectors, ever since Firebug have not shown "empty" text nodes. However some empty text nodes do matter, and they matter a lot (pretty much all web devs have had head scratching moments when trying to get rid of empty space between a series of inline blocks in a line box).

So, a few years back, we innovated in Firefox DevTools and started showing the empty text nodes that do matter in the layout, and that's great because suddenly you could see right in the Inspector all the nodes that did participate in layout.

But to achieve this, we need to check whether a given node actually has a non-0 size in the layout tree of the page, and getBoxQuads is great for this!

If there is a way we can get that information without forcing the layout engine to create frames it would have normally optimized away, then that'd be great. We certainly want to retain that feature, but changing the page once devtools opens up is something we absolutely need to avoid doing.

Comment 3

[Gabriel Luong [:gl] (ΦωΦ)]

Like Razvan has mentioned, the main culprit is here https://searchfox.org/mozilla-central/source/devtools/server/actors/inspector/utils.js#132. We call nodeHasSize() for every text nodes to show an indicator in the markup view if the whitespace text node has an impact on the layout.

One quick fix would be to set a pref for this check meaning the platform team can turn this off. It would mean the platform team would need to know about some obscure prefs for their development in the future. How would people feel about that?

Comment 4

[Emilio Cobos Álvarez (:emilio)]

It should be pretty trivial to add a ChromeOnly option to getBoxQuads() or a ChromeOnly method to avoid suppressing whitespace text-nodes...

Comment 5

[Brad Werth [:bradwerth] | Out until Feb 2025]

(In reply to Emilio Cobos Álvarez (:emilio) from comment #4)

It should be pretty trivial to add a ChromeOnly option to getBoxQuads() or a ChromeOnly method to avoid suppressing whitespace text-nodes...

I'm happy to attempt this and I want to make sure I understand. Is it sufficient for this new method/param to avoid the work in nsCSSFrameConstructor::EnsureFrameForTextNodeIsCreatedAfterFlush? If that's the case, then it seems one way to accomplish this would to be temporarily clear the NS_CREATE_FRAME_IF_NON_WHITESPACE flag on the text nodes, then restore it afterwards. Would that be an acceptable way to handle this?

Comment 6

[Brad Werth [:bradwerth] | Out until Feb 2025]

(In reply to Brad Werth [:bradwerth] from comment #5)

it seems one way to accomplish this would to be temporarily clear the NS_CREATE_FRAME_IF_NON_WHITESPACE flag on the text nodes, then restore it afterwards. Would that be an acceptable way to handle this?

Ugh, I just realized this would avoid creating the frame for that specific node but would still create the frames for all the others. So my next idea would be to annotate the presShell somehow to avoid the call to EnsureFrameForTextNodeIsCreatedAfterFlush in the first place. Thoughts?

Comment 7

[Brad Werth [:bradwerth] | Out until Feb 2025]

Attachment: Bug 1567310 Part 1: Add a ChromeOnly option for suppressing whitespace frames to getBoxQuads.

Comment 8

[Brad Werth [:bradwerth] | Out until Feb 2025]

Attachment: Bug 1567310 Part 2: Make GeometryUtils::GetBoxQuads suppress whitespace frames when requested.

Depends on D45117

Comment 9

[Brad Werth [:bradwerth] | Out until Feb 2025]

Attachment: Bug 1567310 Part 3: Make all devtools callers of getBoxQuads include option to suppress whitespace frames.

Depends on D45118

Comment 10

[Brad Werth [:bradwerth] | Out until Feb 2025]

(In reply to Brad Werth [:bradwerth] from comment #6)

So my next idea would be to annotate the presShell somehow to avoid the call to EnsureFrameForTextNodeIsCreatedAfterFlush in the first place. Thoughts?

As I implemented this, I realized it wasn't hard to just pass through the new option down to the relevant methods, so there's no need to annotate the PresShell. I'll include Emilio in the review when the patches are ready.

No test yet because I don't have a test case. needinfo'ing gabe in the hope he can give me some Steps to Reproduce.

Comment 11

[Gabriel Luong [:gl] (ΦωΦ)]

I imagine Emilio would have the most recent example dealing with this problem. Passing the needinfo for STR over to him.

Comment 12

[Emilio Cobos Álvarez (:emilio)]

Comment 0 mentions a fixed version of the issue. Bug 1501361 is a still-open one.

This is generally hard to test since suppressed white-space nodes are supposed to be indistinguishable of course (modulo bugs).

Comment 13

[Brad Werth [:bradwerth] | Out until Feb 2025]

I've confirmed that the updated patches resolve the issue shown in the Bug 1501361 testcase. I'll figure out how to turn that testcase into a test, add that, and seek review on the patches + test.

Comment 14

[Brad Werth [:bradwerth] | Out until Feb 2025]

Attachment: Bug 1567310 Part 4: Add a test that whitespace frames are not created by opening the inspector.

Depends on D45120

Comment 15

[Brad Werth [:bradwerth] | Out until Feb 2025]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=7c33f5065883fdf8d2716b138b4b221d72001296

Comment 16

[Brad Werth [:bradwerth] | Out until Feb 2025]

I've confirmed that the test built from the testcase for Bug 1501361 passes with these patches applied, and fails when they are not applied. Ready for review.

Comment 17

[Brad Werth [:bradwerth] | Out until Feb 2025]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=3d781d997d735e47362732ff4c61d8330aee9dcd

Comment 18

[Brad Werth [:bradwerth] | Out until Feb 2025]

We've had some discussion about the general intent of these patches in the reviews on https://phabricator.services.mozilla.com/D45257 that I'm summarizing here:

1. This bug exists because our Layout engine suppresses whitespace nodes over-enthusiastically, and fixing that issue is outside the scope of this bug.
2. When getBoxQuads is called on a container element, this suppression is categorically turned off for all whitespace nodes within the container, whether or not those nodes were correctly suppressed in the first place. When devtools is the caller, and one of these nodes has been incorrectly suppressed, it reflows the page which looks bad and makes the tools harder to use.
3. If devtools ever were to call getBoxQuads directly on a whitespace node that has been incorrectly suppressed, we STILL don't want to "fix" the behavior, because of issue 2, above. It's fine for that node to have a no-extent bounds.
4. One way to fix this is to never forcibly unsuppress whitespace nodes.

So this patch should be designed to make sure that devtools continues whitespace suppression choices made by layout, whether they are correct or not. Boris, do you agree?

Comment 19

[Emilio Cobos Álvarez (:emilio)]

(In reply to Brad Werth [:bradwerth] from comment #18)

1. This bug exists because our Layout engine suppresses whitespace nodes over-enthusiastically, and fixing that issue is outside the scope of this bug.

FWIW, this is not quite true: the suppression is (usually) correct, it just happens to be that unsuppressing the frame fixes the layout bug, but the layout bug would exist if there was no text node at all.

So this patch should be designed to make sure that devtools continues whitespace suppression choices made by layout, whether they are correct or not. Boris, do you agree?

The key here is that this is technically observable by content (whether it's a problem in practice I don't know). When one of these frames are unsuppressed, all the frames are unsuppressed, to avoid terrible perf. Though not suppressing whitespace text nodes has a perf impact on its own (which is pretty sad :/).

Comment 20

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/36f0ad0cf7b6
Part 1: Add a ChromeOnly option for suppressing whitespace frames to getBoxQuads. r=bzbarsky
https://hg.mozilla.org/integration/autoland/rev/d6f06c434028
Part 2: Make GeometryUtils::GetBoxQuads suppress whitespace frames when requested. r=emilio
https://hg.mozilla.org/integration/autoland/rev/449301d2ef12
Part 3: Make all devtools callers of getBoxQuads include option to suppress whitespace frames. r=gl
https://hg.mozilla.org/integration/autoland/rev/8da7fe15ba22
Part 4: Add a test that whitespace frames are not created by opening the inspector. r=gl

Comment 21

[Boris Zbarsky [:bzbarsky]]

I think the approach taken here is fine, but we should consider a spec change to this API so we wouldn't have to unsuppress at all, if that's possible. Especially since no one else has implemented this API at all.

Comment 22

[Noemi Erli[:noemi_erli]]

https://hg.mozilla.org/mozilla-central/rev/36f0ad0cf7b6
https://hg.mozilla.org/mozilla-central/rev/d6f06c434028
https://hg.mozilla.org/mozilla-central/rev/449301d2ef12
https://hg.mozilla.org/mozilla-central/rev/8da7fe15ba22