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

RESOLVED FIXED in Firefox 67

Status

()

defect
P1
normal
RESOLVED FIXED
4 months ago
3 months ago

People

(Reporter: dholbert, Assigned: dholbert)

Tracking

Trunk
mozilla67
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox67 fixed)

Details

Attachments

(3 attachments)

Assignee

Description

4 months ago
Posted 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

Updated

4 months ago
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(...))

Assignee

Comment 2

4 months ago

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

Assignee

Updated

4 months ago
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)
Assignee

Comment 4

4 months ago

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.

Assignee

Comment 6

4 months ago

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

Assignee

Updated

4 months ago
Blocks: 1526097

Comment 10

4 months ago
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

Comment 11

4 months ago
bugherder
Status: ASSIGNED → RESOLVED
Closed: 4 months 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
Upstream PR merged

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

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.