793456 - nsLayoutUtils::GetFirstLinePosition() for a button-frame should return the button's inner-block's first-line.

Status: ASSIGNED

(Core :: Layout, defect)

Description

[Daniel Holbert [:dholbert]]

Right now, nsLayoutUtils::GetFirstLinePosition() [1] generally expects to be given a block, but it has special cases for handling tables, scroll-frames, and fieldset frames.

I use this method to query children for their first-line baseline, for baseline alignment in flexbox (bug 666041), but it doesn't work for buttons right now.  (They hit the "no baseline" failure case")

We can easily make it work for buttons -- we just want to make a recursive call, passing in our first child (the block-frame that holds the button's contents). Conveniently, the fieldset case already does this -- we just need to make buttons fall into that case as well.



[1] ...and GetFirstLineBaseline(), which relies on GetFirstLinePosition()

Comment 1

[Daniel Holbert [:dholbert]]

Attachment: fix

Here's the patch.  A try run from a few days ago indicates that this doesn't affect current reftests, at least on Linux/Mac.  (the platforms I ran it on)
  https://tbpl.mozilla.org/?tree=Try&rev=f73448ffc689

Comment 2

[Daniel Holbert [:dholbert]]

(My flexbox reftests patch includes some reftests that exercise this code (& depend on this change), too.)

Comment 3

[Daniel Holbert [:dholbert]]

For reference / context, here's the larger region of code that this patch touches:
https://mxr.mozilla.org/mozilla-central/source/layout/base/nsLayoutUtils.cpp?mark=3358-3401#3358

(The patch just makes buttons drop into the "nsGkAtoms::fieldSetFrame" clause, and adds a comment explaining why that's reasonable.)

Comment 4

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 663780 [details] [diff] [review]
fix

r=dbaron

Comment 5

[JP Rosevear [:jpr]]

Is there any reason not to land this?

Comment 6

[Daniel Holbert [:dholbert]]

Sorry -- around when this got r+, I looked into adding a testcase & making sure this patch actually changed behavior before landing, but I couldn't get it to have any effect anymore.  

I'm not sure why -- it might be because I changed something else in the flexbox code that makes it unnecessary, but regardless, I didn't want to land it without being sure that it was useful & tested.

It's been on my list to "dig into & either land it or figure out why it matter", but I haven't gotten to it yet.  Thanks for the reminder -- I'll try to get to it before long.

Comment 7

[Daniel Holbert [:dholbert]]

(In reply to Daniel Holbert [:dholbert] from comment #6)
> It's been on my list to "dig into & either land it or figure out why it
> matter", but I haven't gotten to it yet.

er "why it doesn't matter"