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

RESOLVED FIXED in Firefox 67

Status

()

P1
normal
RESOLVED FIXED
15 days ago
9 days ago

People

(Reporter: dholbert, Assigned: dholbert)

Tracking

Trunk
mozilla67
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox67 fixed)

Details

Attachments

(3 attachments)

(Assignee)

Description

15 days ago

Created attachment 9041831 [details]
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

15 days 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

14 days 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

14 days 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

14 days 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

14 days ago

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

(Assignee)

Comment 7

14 days ago

Created attachment 9042315 [details]
Bug 1525628 part 1: Add nsIFrame::IsBlockFrameOrSubclass() convenience accessor. r?mats

(Assignee)

Comment 8

14 days ago

Created attachment 9042316 [details]
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

(Assignee)

Updated

14 days ago
Blocks: 1526097

Comment 10

13 days 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

13 days ago
bugherder
Status: ASSIGNED → RESOLVED
Last Resolved: 13 days ago
status-firefox67: affected → fixed
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.

You need to log in before you can comment on or make changes to this bug.