1672640 - Misplaced dropdown arrow on office/outlook calendar

Status: RESOLVED FIXED

(Core :: Layout, defect)

Description

[Anca Soncutean, Desktop QA]

Attachment: Fx vs Chrome screenshot.png

Affected versions

• 82.0
• 83.0b2
• 84.0a1 (2020-10-22)

Affected platforms

• Ubuntu 18.04
• Windows 10

Steps to reproduce

1. Go to https://outlook.live.com/calendar/0/view/month
2. Enter valid credentials
3. Click on any day tile
4. Observe the Calendar dropdown arrow

Expected result

• Calendar dropdown arrow is aligned with the label

Actual result

• Calendar dropdown arrow is misplaced

Regression range

• First bad: 1aa53285261e17aa6a8b36e05c7208d616c24aa4
• Last good: 3a38af2b7e83456941bed532016ae31208589dd0
• Pushlog: https://hg.mozilla.org/integration/autoland/pushloghtml?fromchange=3a38af2b7e83456941bed532016ae31208589dd0&tochange=1aa53285261e17aa6a8b36e05c7208d616c24aa4
• Potential regressor: 1492538

Suggested severity

• S4

Comment 1

[Sean Voisen (:svoisen)]

Calling it an S3 since it affects Outlook.

Ting-Yu: Any ideas?

Comment 2

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

It would be super helpful if someone having an Outlook account could attached a reduced testcase.

Comment 3

[Ryan VanderMeulen [:RyanVM]]

While I understand that the lack of a reduced testcase doesn't make debugging this any easier, this is a pretty obvious visual regression on a major site. Can we try to prioritize this for 85?

Comment 4

[Daniel Holbert [:dholbert]]

I can reproduce (I've got the free Microsoft/live.com account, which seems to work as a way to log in and see a free Outlook calendar).

(Observation: sometimes while poking around in devtools, the misaligned arrow realigns itself, i.e. the bug goes away. This makes sense for a regression from bug 1492538, where there's some sort of invalidation or flush caused by devtools or a viewport-resize or both.)

I'll take a look here.

Comment 5

[Daniel Holbert [:dholbert]]

Minor update: I'm still intending to look at this, but I may not get to this until after the soft freeze, since I'm trying to finish up some printing work that needs to make it in for 85. I'm hoping that a fix here will be relatively minimal impact safe for uplift after that point, so that we can still address this in 85 that way.

(Worst-case, if the fix doesn't make it into the 85 release, we can probably live with the regression for a few weeks longer since we've been apparently shipping with it since 80, based on when regressing bug 1492538 landed.)

Comment 6

[Daniel Holbert [:dholbert]]

Attachment: testcase 1 (partially reduced, using testcase reducer addon)

Comment 7

[Daniel Holbert [:dholbert]]

Attachment: testcase 2 (further reduced, manually)

Comment 8

[Daniel Holbert [:dholbert]]

Expected behavior for testcase 1 is for the text characters to all be baseline-aligned. ("Calendar"/"v" in the first testcase, and "abc"/"def" in the second testcase.)

Comment 9

[Daniel Holbert [:dholbert]]

If I give FlexItem::NeedsFinalReflow an early return true statement (forcing us to take the slow path), then these testcases give expected results (not too surprising).

Comment 10

[Daniel Holbert [:dholbert]]

So approximately what's happening here is:
(1) We're able to optimize away the final reflow of the nsTextControlFrame flex item, using our optimization for "the frame's not dirty and conditions haven't changed".
(2) However, we do need to know its baseline (to establish the baseline of its flex container), so we call FlexItem::ResolvedAscent to determine that, which calls nsLayoutUtils::GetFirstLineBaseline, which calls nsLayoutUtils::GetFirstLinePosition.
(3) The call to GetFirstLinePosition fails (i.e. returns false) here:
https://searchfox.org/mozilla-central/rev/6a023272d590409c80458d373986e379b3ad86f4/layout/base/nsLayoutUtils.cpp#5744-5745
...because we don't have a case statement for its frame type in the listing above.
(4) So we end up taking the fallback path in FlexItem::ResolvedAscent, which synthesizes a baseline from the border-box (i.e. we use the border-bottom edge as our baseline).

tl;dr: we're leaning on nsLayoutUtils::GetFirstLineBaseline to give us an answer here -- but it fails for nsTextControlFrame returns false which (incorrectly) indicates that there's no baseline.

Comment 11

[Daniel Holbert [:dholbert]]

Attachment: Bug 1672640: Use GetNaturalBaselineBOffset to ask flex items what their baseline is.

Comment 12

[Daniel Holbert [:dholbert]]

The attached patch (just trying one additional more-direct "please give me your baseline" API) fixes both testcases here as well as the testcases on see-also bug 1686961 [EDIT: just kidding, this doesn't help with bug 1686961's testcases after all -- a different diagnostic-patch in my stack was helping with those in my local test]

Comment 13

[Brian Grinstead [:bgrins]]

Doesn't seem like this will make 87, but keeping it on the radar for 88

Comment 14

[Pascal Chevrel:pascalc (PTO until April 26)]

Daniel, do you have an update here? Thanks

Comment 15

[Daniel Holbert [:dholbert]]

No updates. Still on my to-do list, still hasn't risen to the top, still hoping to get to it soon.

Comment 16

[Daniel Holbert [:dholbert]]

Attachment: Bug 1672640 part 1: Flatten logic and update comments in FlexItem::ResolvedAscent(). r=TYLin

This patch doesn't affect behavior; it just refactors some logic to have an
early-return and reduce indentation, to make the next patch in this series
easier/simpler.

While we're at it, this patch also updates & extends some neighboring
code-comments to be more specific & more correct about how this code behaves
and its limitations.

Comment 17

[Daniel Holbert [:dholbert]]

Attachment: Bug 1672640 part 2: Fall back to GetNaturalBaselineBOffset to ask flex items what their baseline is. r=TYLin

Our earlier call to nsLayoutUtils::GetFirstLineBaseline/GetLastLineBaseline
works in most cases, but those APIs don't handle every frame type and fails for
text control frames (for example). This new call should handle those cases
by directly asking the frame for its baseline.

Depends on D121921

Comment 18

[Daniel Holbert [:dholbert]]

Attachment: Bug 1672640 part 3: Adjust GetNaturalBaselineBOffset implementations to bail (report no baseline) if the frame has 'contain:layout'. r?TYLin

Some of our GetNaturalBaselineBOffset implementations already have this; others don't. But they all should have it, or else a caller might improperly query their baseline and use it for layout despite the frame having 'contain:layout'.

Without this patch, the rest of this patch-stack makes us fail WPT test
contain-layout-suppress-baseline-002.html because we improperly honor the
baseline for the 'contain:layout' buttons at the top of the test.

Depends on D121922

Comment 19

[Daniel Holbert [:dholbert]]

I think this is relatively low-risk (and just a bug fix, not a new feature or a pref-flip or anything of that sort); so I'm going ahead & landing during the soft-freeze.

Comment 20

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/f95806b5b69b
part 1: Flatten logic and update comments in FlexItem::ResolvedAscent(). r=TYLin
https://hg.mozilla.org/integration/autoland/rev/a0f6f8d6feda
part 2: Fall back to GetNaturalBaselineBOffset to ask flex items what their baseline is. r=TYLin
https://hg.mozilla.org/integration/autoland/rev/de6bd6b6def5
part 3: Adjust GetNaturalBaselineBOffset implementations to bail (report no baseline) if the frame has 'contain:layout'. r=TYLin

Comment 21

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

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

Comment 22

[Sandor Molnar[:smolnar]]

https://hg.mozilla.org/mozilla-central/rev/f95806b5b69b
https://hg.mozilla.org/mozilla-central/rev/a0f6f8d6feda
https://hg.mozilla.org/mozilla-central/rev/de6bd6b6def5

Comment 23

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

Upstream PR merged by moz-wptsync-bot

Comment 24

[BugBot [:suhaib / :marco/ :calixte]]

Since the status are different for nightly and release, what's the status for beta?
For more information, please visit auto_nag documentation.

Comment 25

[Ryan VanderMeulen [:RyanVM]]

I'm kinda-sorta wondering if we're going to want this on ESR91 since it'll be a user-facing regression for enterprises updating from ESR78 to ESR91 and this feels like the kind of site they'd be likely to use. What are your thoughts, Dan?

Comment 26

[Daniel Holbert [:dholbert]]

That's a fair point - I agree with your reasoning. When does ESR91 go out? Is that tomorrow like regular release 91?

I'd be a little uneasy about uplifting this straight to release (whether ESR or not) right now, given that it only just landed on Nightly a few days back, and it's not quite zero-risk. But I think it's pretty low-risk, and I agree that there's likely an overlap between Office/Outlook users and ESR users, and you make a good point about this being a regression with respect to 78.

(Sorry this took so long to close out... as hinted at in comment 12, I thought this bug and a few other bugs were all versions of the same core issue, and I was hesitant to land the tentative patch here without having gotten to the bottom of the other issues as well. Fortunately TYLin took care of the other issues & they turned out to be unrelated/distinct bugs.)

Comment 27

[Julien Cristau [:jcristau] (back April 22)]

91.0esr does go out today, but we don't update users from ESR78 to 91 until the 91.3 release in November, so we have some space to take this in 91.1 or 91.2 if we want.

Comment 28

[Daniel Holbert [:dholbert]]

Comment on attachment 9234979 [details]
Bug 1672640 part 1: Flatten logic and update comments in FlexItem::ResolvedAscent(). r=TYLin

ESR Uplift Approval Request

• If this is not a sec:{high,crit} bug, please state case for ESR consideration: Visual regression [with respect to ESR78] in enterprise-relevant site (Outlook calendar)
• User impact if declined: Misaligned arrow in Outlook calendar's custom dropdown-menu-widget; see screenshot in comment 0.
• Fix Landed on Version: 92
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): This patch-stack makes our code for baseline alignment of flex items slightly more robust.

Specifically, this patch-stack changes how we behave when we fail to find the baseline for a given flex item, during our incremental-layout optimized codepath. This patch stack makes us ask the item again, in a different way, which succeeds in some cases where the previous approach would fail.

This represents a small step in the direction of correctness/consistency, so it feels fairly low-risk.

• String or UUID changes made by this patch: None

Comment 29

[Daniel Holbert [:dholbert]]

(In reply to Julien Cristau [:jcristau] from comment #27)

91.0esr does go out today, but we don't update users from ESR78 to 91 until the 91.3 release in November, so we have some space to take this in 91.1 or 91.2 if we want.

Thanks, Julien. In that case, I think this would be an excellent uplift-candidate for 91.1esr. I've gone ahead and requested ESR approval (with the intent being that this would land for the 91.1esr cycle, after 91.0esr goes out today)

Comment 30

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 9234979 [details]
Bug 1672640 part 1: Flatten logic and update comments in FlexItem::ResolvedAscent(). r=TYLin

Approved for 91.1esr.

Comment 31

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-esr91/rev/efba45b1f916
https://hg.mozilla.org/releases/mozilla-esr91/rev/c4b852a855cf
https://hg.mozilla.org/releases/mozilla-esr91/rev/9e7a70ee06f0