Closed Bug 1468416 Opened 6 years ago Closed 6 years ago

CSS Grid highlighter does not work on <fieldset> elements

Categories

(DevTools :: Inspector, defect, P2)

Product:
DevTools
Component:
Inspector
Type:
defect

Tracking

(firefox62 fixed)

Status:
RESOLVED FIXED
Milestone:
Firefox 62
Tracking Flags:
Tracking Status
firefox62 --- fixed

People

(Reporter: pbro, Assigned: bradwerth)

References

(Blocks 1 open bug)

Details

Attachments

(2 files)

Bug 1468416 Part 1: Make nsGridContainerFrame::GetGridFrameWithComputedInfo use the same code to find the grid container frame as used by the layout algorithm itself.
59 bytes, text/x-review-board-request
MatsPalmgren_bugz
: review+
Details
Bug 1468416 Part 2: Update a grid API test to check fieldsets and buttons.
59 bytes, text/x-review-board-request
MatsPalmgren_bugz
: review+
Details 
STR:
- open the following URL:
data:text/html,<fieldset style="display:grid;grid-template-columns: auto 1fr;grid-gap: 1em;"><legend>a fieldset</legend><label for="name">name</label><input id="name"></fieldset>
- open the inspector and switch to the Layout sidebar
- click on the checkbox next to the <fieldset> element in the "Grid" section

Expected: the grid should be highlighted
Actual: it isn't!
Doing some debugging, I see that:
el.getGridFragments() returns an empty array for the element, even if the display type on el is indeed grid.

CC'ing Brad to make sure this shows up on his radar.
Assignee: nobody → bwerth
Product: Firefox → DevTools
Talking to dholbert, it seemed like it would also fail for buttons, but apparently it does:

data:text/html,<button style="display:grid;grid-template-rows:1fr 1fr;height:50px;grid-gap:1em;"><span style="grid-row:2">test</span></button>
*but apparently it does work
Looks like it works in buttons because GetContentInsertionFrame is implemented there (and forwards to the anonymous wrapper frame inside the button):
https://dxr.mozilla.org/mozilla-central/rev/75a32b57132f8cba42779555662a057a0416a313/layout/forms/nsHTMLButtonControlFrame.h#90-93

...but GetContentInsertionFrame does *not* currently have a custom implementation in fieldset:
https://dxr.mozilla.org/mozilla-central/rev/75a32b57132f8cba42779555662a057a0416a313/layout/forms/nsFieldSetFrame.h

So that explains the difference.  Perhaps we need to implement that for nsFieldSetFrame, or (gross) add a special case for fieldsets at the spot where we call GetContentInsertionFrame in the grid "GetGridFrameWithComputedInfo" function:
https://dxr.mozilla.org/mozilla-central/rev/75a32b57132f8cba42779555662a057a0416a313/layout/generic/nsGridContainerFrame.cpp#7020
Attachment #8985506 - Flags: review?(dholbert)
I think we need to do a brief audit of GetContentInsertionFrame() callers, to see how this might change behavior (via how GetContentInsertionFrame is used).  I suspect all behavior-changes will be for the better, but it'd be good to test potentially-affected scenarios & make sure nothing crashes or does something unexpected.

We may also find some cleanup possibilities, too -- when starting to look at callers, I found this one:
https://dxr.mozilla.org/mozilla-central/rev/75a32b57132f8cba42779555662a057a0416a313/layout/base/RestyleManager.cpp#1450-1451
...where I suspect this we'll be able to remove the frame->IsFieldSetFrame() special case, since fieldsets will now pass the subsequent "|| frame->GetContentInsertionFrame() != frame" check.
I notice that we have some related stuff here, "GetFieldSetBlockFrame":

https://dxr.mozilla.org/mozilla-central/rev/75a32b57132f8cba42779555662a057a0416a313/layout/base/nsCSSFrameConstructor.cpp#409-416

Looks like that function was added in bug 508665, and iterated on in bug 931464.

Mats, you were involved on both of those -- do you remember why we don't have GetContentInsertionFrame() already doing what Brad's patch makes it do? (forwarding to the main child)
Flags: needinfo?(mats)
(In reply to Daniel Holbert [:dholbert] from comment #7)
> I think we need to do a brief audit of GetContentInsertionFrame() callers,
> to see how this might change behavior (via how GetContentInsertionFrame is
> used).  I suspect all behavior-changes will be for the better, but it'd be
> good to test potentially-affected scenarios & make sure nothing crashes or
> does something unexpected.

From reading some of the other callsites, it seems that nsFieldSetFrames have TWO content insertion frames. Although it's useful to return one of them from GetContentInsertionFrame, it's not strictly accurate.

There's a narrower fix I can roll out. nsGridContainerFrame::GridItemInfo::InnerFrame is an inline static method https://searchfox.org/mozilla-central/source/layout/generic/nsGridContainerFrame.cpp#642 which finds the frame that has the display:grid property applied. The use of GetContentInsertionFrame() is just trying to approximate this. I can uplift the InnerFrame method to nsGridContainerFrame and expose it via the header, and then we'll be guaranteed to be following the same frames that the grid layout algorithm is using.
Attachment #8985506 - Flags: review?(dholbert) → review?(mats)
Moved review over to Mats since the code is now entirely in nsGridContainerFrame.
(In reply to Daniel Holbert [:dholbert] from comment #8)
> Mats, you were involved on both of those -- do you remember why we don't
> have GetContentInsertionFrame() already doing what Brad's patch makes it do?
> (forwarding to the main child)

I seem to recall that the frame constructor depended on nsFieldSetFrame
returning itself as the GetContentInsertionFrame() when inserting a <legend>.
(see GetAdjustedParentFrame and related code)
But perhaps it doesn't anymore.

Note that it's not actually supposed to *use* that frame for anything though:
https://searchfox.org/mozilla-central/rev/42930ab9634ebf3f62aed60f7d1c1bf25c0bf00c/layout/forms/nsFieldSetFrame.cpp#612-642
It was probably just for testing IsFieldSetFrame() in some place
and then manually doing the frame insertion with the right parent.

In most cases I would expect forwarding to the inner frame to be
the right thing to do, including this case.  But changing this
requires careful analysis of how the frame constructor handles
these things nowadays.

> I can uplift the InnerFrame method to nsGridContainerFrame and expose it via the header

Yeah, that's probably a better (less risky) short-term fix for now.
Flags: needinfo?(mats)
Comment on attachment 8985506 [details]
Bug 1468416 Part 1: Make nsGridContainerFrame::GetGridFrameWithComputedInfo use the same code to find the grid container frame as used by the layout algorithm itself.

https://reviewboard.mozilla.org/r/251106/#review257404

::: layout/generic/nsGridContainerFrame.h:249
(Diff revision 2)
> +   * Return a containing grid frame for the supplied frame, if available.
> +   * @return nullptr if aFrame has no grid container.

nit: s/containing grid frame/grid container frame/

::: layout/generic/nsGridContainerFrame.h:252
(Diff revision 2)
>  
>    /**
> +   * Return a containing grid frame for the supplied frame, if available.
> +   * @return nullptr if aFrame has no grid container.
> +   */
> +  static nsGridContainerFrame* GetGridFrame(nsIFrame* aFrame);

nit: I'd prefer to spell it out as GetGridContainerFrame rather than GetGridFrame

::: layout/generic/nsGridContainerFrame.cpp:559
(Diff revision 2)
> -    nsIFrame* innerFrame = InnerFrame(mFrame);
> -    if (innerFrame->IsGridContainerFrame()) {
> +    nsGridContainerFrame* gridFrame = GetGridFrame(mFrame);
> +    if (gridFrame) {

Using the shorter "gridFrame" variable name seems clear enough here though...
Attachment #8985506 - Flags: review?(mats) → review+
Attachment #8985649 - Flags: review?(mats)
Comment on attachment 8985649 [details]
Bug 1468416 Part 2: Update a grid API test to check fieldsets and buttons.

https://reviewboard.mozilla.org/r/251192/#review258314

It's probably worth testing a div/fieldset/button that has overflow:hidden too.
Attachment #8985649 - Flags: review?(mats) → review+
Pushed by bwerth@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/55e1e865e626
Part 1: Make nsGridContainerFrame::GetGridFrameWithComputedInfo use the same code to find the grid container frame as used by the layout algorithm itself. r=mats
https://hg.mozilla.org/integration/autoland/rev/f6f3c67e4d33
Part 2: Update a grid API test to check fieldsets and buttons. r=mats
Ugh. My refactoring in Part 1 failed to continue support for the nullptr aFrame case, and the tests I ran were not broad enough to catch these failures. The updated patches should resolve it, and I'm running the right tests now. I will attempt to re-land after try run comes back successful.
Flags: needinfo?(bwerth)
Pushed by bwerth@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/ad40d4b300a6
Part 1: Make nsGridContainerFrame::GetGridFrameWithComputedInfo use the same code to find the grid container frame as used by the layout algorithm itself. r=mats
https://hg.mozilla.org/integration/autoland/rev/f63604ec72d1
Part 2: Update a grid API test to check fieldsets and buttons. r=mats
https://hg.mozilla.org/mozilla-central/rev/ad40d4b300a6
https://hg.mozilla.org/mozilla-central/rev/f63604ec72d1
Status: NEW → RESOLVED
Closed: 6 years ago
status-firefox62: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 62
Flags: qe-verify-
See Also: → 1648774
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: