Buttons with display: table-cell wrap unexpectedly when Inspector is opened

NEW
Unassigned
(NeedInfo from)

Status

()

Core
Layout
7 months ago
4 months ago

People

(Reporter: cnthndlthtrth, Unassigned, NeedInfo)

Tracking

(Depends on: 1 bug, {regression, testcase})

52 Branch
regression, testcase
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox51 unaffected, firefox52+ fix-optional, firefox53+ wontfix, firefox54+ fix-optional, firefox55 ?)

Details

Attachments

(2 attachments, 2 obsolete attachments)

(Reporter)

Description

7 months ago
Created attachment 8835098 [details]
display_table_cell_wrapping_issue.html

User Agent: Mozilla/5.0 (Windows NT 6.2; WOW64; rv:52.0) Gecko/20100101 Firefox/52.0
Build ID: 20170202101509

Steps to reproduce:

On page with buttons' display set to table-cell and width 50%, when the Inspector is opened the buttons stack instead of sitting next to each other. 


Actual results:

Open attached HTML document, right click on page to open Inspector. Buttons are re-positioned.


Expected results:

Open attached HTML document, right click on page to open Inspector. Inspector opens without changing page layout.

Comment 1

6 months ago

https://hg.mozilla.org/integration/fx-team/pushloghtml?fromchange=687572ea20cb84f1842ad00dcc8ca1712868a361&tochange=fe1d7c28668f0eb07c2f91bd0b3b0fa635d82387
Blocks: 1304685
Status: UNCONFIRMED → NEW
status-firefox51: --- → unaffected
status-firefox52: --- → affected
status-firefox53: --- → affected
status-firefox54: --- → affected
tracking-firefox52: --- → ?
tracking-firefox53: --- → ?
tracking-firefox54: --- → ?
Component: Untriaged → Developer Tools: Inspector
Ever confirmed: true
Flags: needinfo?(pbrosset)
Keywords: regression
OS: Unspecified → All
Hardware: Unspecified → All
Setting this as P1, Patrick please fix this!
Priority: -- → P1
Had a quick look, and the layout changes as soon as we are retrieving getBoxQuads on the text node between the two buttons on the test page. The call comes from the inspector actor nodeHasSize() [1]

If you early return false here to bypass the issue, you can still trigger it by running 

> document.querySelector('.disp-table').childNodes[2].getBoxQuads()

on the page. So it looks like this is an issue with getBoxQuads.

[1] http://searchfox.org/mozilla-central/rev/d3307f19d5dac31d7d36fc206b00b686de82eee4/devtools/server/actors/inspector.js#3107
Great investigation Julian, thanks!

My first idea was to look at the highlighter's CSS, thinking that it could be the only one having this kind of impact on a content page.

Anyway, you're right, running document.querySelector('.disp-table').childNodes[2].getBoxQuads() on the page (without devtools open) does produce the same bug.
Indeed, bug 1304685 introduced this bug because it made use of getBoxQuads on text nodes.

So, new STR:
- open https://bugzilla.mozilla.org/attachment.cgi?id=8835098
- open the console (ctrl+shift+K) (make sure you don't open the inspector panel),
- execute the following script:
document.querySelector('.disp-table').childNodes[2].getBoxQuads()

@mats: could you or someone investigate this issue and confirm if this is an issue related to getBoxQuads?
Flags: needinfo?(pbrosset) → needinfo?(mats)
Tracking for 52 as new regression.
tracking-firefox52: ? → +
tracking-firefox53: ? → +
tracking-firefox54: ? → +

Comment 6

6 months ago
With the new STR in comment 4, probably we are able to find out the bug that caused regression for more insight. Loic, could you help on that ? Thanks.
Flags: needinfo?(epinal99-bugzilla2)

Comment 7

6 months ago
With STR from comment #4, the bug is here since the implementation of the getBoxQuads API in Firefox 31 (bug 917755).
Flags: needinfo?(epinal99-bugzilla2)

Comment 8

6 months ago
Hi :jet,
Could you help find someone to look at this one if :mats is busy on other priority?
Flags: needinfo?(bugs)

Comment 9

6 months ago
Before opening the devtools inspector we have this frame tree (simplified):
TableCell(div)(1)@7fffb4142958 {0,0,18000,1860}
  Block(div)(1)@7fffb4142dc0 {0,0,18000,1860}
      HTMLButtonControl(button)
      HTMLButtonControl(button)

and after we have:

TableCell(div)(1)@7fffb4142958 {0,0,18000,3180}
  Block(div)(1)@7fffb416ae18 {0,0,18000,3180}
      Text(0)"\n          " {0,1200,0,0}
      HTMLButtonControl(button)
      Text(2)"\n          " {9000,300,0,1140}
      HTMLButtonControl(button)(3)
      Text(4)"\n      " {9000,1920,0,1140}

I suspect that calling .getBoxQuads() on the whitespace nodes
that we normally suppress creating frames for inside the table
magically creates the frames and changes the layout.

From the regression range in comment 1:
https://hg.mozilla.org/integration/fx-team/rev/fe1d7c28668f#l12.70
Flags: needinfo?(mats)
Created attachment 8839304 [details]
testcase
Boris, I seem to recall we discussed what .getBoxQuads() should do on whitespace
nodes with suppressed frames inside tables, but I can't find it in Bugzilla.
Do you recall if this is intentional behavior or not?
Flags: needinfo?(bzbarsky)
Keywords: testcase
These suppressed frames are not "inside tables" in any meaningful way.  They're inside a table cell.

The frame tree with the text suppressed looks buggy to me.  We _shouldn't_ be suppressing in that case, for the whitespace between the buttons, because we're looking at whitespace between two random boxes that are kids of a table cell.  It's possible that we mess this up because of the (otherwise ignored at the moment) "display: table-cell" on the <button> elements...  The rendering should end up the same (at the moment) whether they're styled "table-cell" or "inline-block", and to the extent that it does not that's a bug.

The "right" rendering here would be to actually make the buttons into table cells (as in, put them in different table columns) and suppress the whitespace entirely, but I'm not sure anyone does that.

In Chrome, it looks like there's some sort of bug along the lines of ours where they put the two buttons next to each other in a single table cell but suppress the whitespace between them.  But that's because their whitespace processing in tables like this is generally buggy and has been forever.  :(
Flags: needinfo?(bzbarsky)
I filed https://bugs.chromium.org/p/chromium/issues/detail?id=694374 but I'm not holding my breath on them fixing it, just like they haven't fixed previous reports along those lines.

Anyway, I think we should fix the incorrect suppression.  That will cause the initial testcase to consistently be on two lines, which is the correct layout given CSS specs plus the "buttons don't actually get to be cells" behavior.
I think the problem is this bit in nsCSSFrameConstructor::AddFrameConstructionItemsInternal (with some comments removed):

    bool isInline =
      ((bits & FCDATA_IS_TABLE_PART) &&
       (!aParentFrame || // No aParentFrame means inline
        aParentFrame->StyleDisplay()->mDisplay == StyleDisplay::Inline)) ||
      display->IsInlineOutsideStyle() ||
      isPopup;

    item->mIsBlock = !isInline &&
                     !display->IsAbsolutelyPositionedStyle() &&
                     !display->IsFloatingStyle() &&
                     !(bits & FCDATA_IS_SVG_TEXT);

That's not good enough in this case.  display:table-cell is not IsInlineOutsideStyle(), but we're also not FCDATA_IS_TABLE_PART so we're going to ignore that display value anyway...

We should probably add 

  (!display->IsInnerTableStyle() || (bits & FCDATA_IS_TABLE_PART))

to the mIsBlock conditions.  Or something along those lines...  As the comments around this code say, setting mIsBlock to true when it's not is _really_ bad; it leads to rendering bugs like this.  Setting it false when it's really true is suboptimal, but not incorrect.

I guess we could be really conservative and replace "!isInline" with "display->IsBlockOutsideStyle()".  But I _think_ just adding the above bit about FCDATA_IS_TABLE_PART would have the same effect with slightly nicer handling of tables.

Updated

6 months ago
Depends on: 1341188
OK, I filed bug 1341188 for the underlying layout bug.
Flags: needinfo?(bugs)
Getting pretty late in the Firefox 52 cycle for a fix. Would take a patch if one appears, but not going to keep tracking this.
status-firefox52: affected → fix-optional
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Attachment #8846539 - Flags: review?(jwatt)
Attachment #8846540 - Flags: review?(hikezoe)
Comment hidden (offtopic)
Comment hidden (offtopic)
Attachment #8846540 - Attachment is obsolete: true
Gabriel, is this still a priority? It isn't assigned to anyone currently and it's marked as P1 for your team.
status-firefox53: affected → wontfix
status-firefox54: affected → fix-optional
status-firefox55: --- → ?
Flags: needinfo?(gl)
Liz, I think this might need to reprioritize and moved to the right component in platform. Forwarding this to jet.
Flags: needinfo?(gl) → needinfo?(bugs)
Untriaged, and moved to the Layout component. Maybe it even needs to be closed as a duplicate of bug 1341188?
Component: Developer Tools: Inspector → Layout
Priority: P1 → --
Product: Firefox → Core
You need to log in before you can comment on or make changes to this bug.