Closed Bug 1525628 Opened 3 years ago Closed 3 years ago

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

Categories

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

defect

Tracking

()

RESOLVED FIXED
mozilla67
Tracking Status
firefox67 --- fixed

People

(Reporter: dholbert, Assigned: dholbert)

References

Details

Attachments

(3 files)

Attached file 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 )

Assignee: nobody → dholbert
Status: NEW → ASSIGNED

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

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.)

Flags: needinfo?(mats)

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.

Flags: needinfo?(mats)

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)

Yeah, sounds good.

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

Blocks: 1526097
Pushed by dholbert@mozilla.com:
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
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla67
Created web-platform-tests PR https://github.com/web-platform-tests/wpt/pull/15324 for changes under testing/web-platform/tests

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.

QA Whiteboard: [good first verify]
You need to log in before you can comment on or make changes to this bug.