1525628 - Make "is block frame" check more nuanced, in nsHTMLScrollFrame::GetLogicalBaseline

Status: RESOLVED FIXED

(Core :: Layout: Scrolling and Overflow, defect, P1)

Description

[Daniel Holbert [:dholbert]]

Attachment: testcase 1

STR:

1. Load testcase
2. Click to expand the details

EXPECTED RESULTS:
"aaa" should align with the border bottom (technically the margin bottom but there's no margin), both before and after it expands.

ACTUAL RESULTS:
"aaa" aligns with the baseline of the text inside the details frame (and with clipped text super-far-down, after it's expanded)

This is a followup to bug 969874 -- see bug 969874 comment 22. Emilio suggested that we want to use IsFrameOfType(eBlock) in nsHTMLScrollFrame::GetLogicalBaseline, and I think that's probably correct.

(The spec text around this isn't specific enough -- it says "if an inline-block is a scroll container" but that's not specific enough per https://github.com/w3c/csswg-drafts/issues/3611 )

Comment 1

[Mats Palmgren (inactive)]

Yeah, my bad, I should have spotted that. I think what we want to use
here is static_cast<nsBlockFrame*>(do_QueryFrame(...))

Comment 2

[Daniel Holbert [:dholbert]]

Any reason to prefer that method vs. emilio's suggestion of IsFrameOfType(eBlockFrame)

(i.e. would you object to IsFrameOfType(eBlockFrame)? That seems like a simpler/more direct way of asking the "is this a block" question, to me.)

Comment 3

[Mats Palmgren (inactive)]

It's faster in the common case (avoids the virtual call).
We should probably remove eBlockFrame now that do_QueryFrame
does this faster. I'm assuming no subclass opts out from
eBlockFrame and they actually are equivalent.

Comment 4

[Daniel Holbert [:dholbert]]

OK, thanks - yeah, I didn't recall for sure whether do_QueryFrame required a virtual call under the hood.

I might add a convenience member-function nsIFrame::IsBlockFrameOrSubclass() to encapsulate that check -- sound good? (name bikeshedding welcome)

Comment 5

[Mats Palmgren (inactive)]

Yeah, sounds good.

Comment 6

[Daniel Holbert [:dholbert]]

(Ah, right, do_QueryFrame is faster now as of bug 1364815.)

Comment 7

[Daniel Holbert [:dholbert]]

Attachment: Bug 1525628 part 1: Add nsIFrame::IsBlockFrameOrSubclass() convenience accessor. r?mats

Comment 8

[Daniel Holbert [:dholbert]]

Attachment: Bug 1525628 part 2: Generalize block-frame special case in scrollframe baseline code, so that it includes block subclasses like `<details>`. r?mats

Depends on D19083

Comment 9

[Daniel Holbert [:dholbert]]

Try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=d7da91cd072d45ed891281c4385eed1dc28a2611

Comment 10

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/91eea845d54e
part 1: Add nsIFrame::IsBlockFrameOrSubclass() convenience accessor. r=mats
https://hg.mozilla.org/integration/autoland/rev/ee8803de7f47
part 2: Generalize block-frame special case in scrollframe baseline code, so that it includes block subclasses like `<details>`. r=mats

Comment 11

[Narcis Beleuzu [:NarcisB]]

https://hg.mozilla.org/mozilla-central/rev/91eea845d54e
https://hg.mozilla.org/mozilla-central/rev/ee8803de7f47

Comment 12

[Web Platform Test Sync Bot [:wpt-sync] (Matrix: #interop:mozilla.org)]

Created web-platform-tests PR https://github.com/web-platform-tests/wpt/pull/15324 for changes under testing/web-platform/tests

Comment 13

[Web Platform Test Sync Bot [:wpt-sync] (Matrix: #interop:mozilla.org)]

Upstream PR merged

Comment 14

[Ting-Yu Lin [:TYLin] (PDT, UTC-7)]

I was late for bikeshedding ;) FWIW, we have nsLayoutUtils::GetAsBlock() for similar purpose.

https://searchfox.org/mozilla-central/rev/00c0d068ece99717bea7475f7dc07e61f7f35984/layout/base/nsLayoutUtils.cpp#4505-4508

Comment 15

[Mats Palmgren (inactive)]

Yeah, I guess nsIFrame::GetAsBlock() could have worked too.
I think should remove the nsLayoutUtils method now though,
so we don't have two ways to do exactly the same thing.