Closed Bug 1606617 Opened 5 years ago Closed 5 years ago

"Add Search Engine" confirmation dialog button and description are overlapped each other

Categories

(Toolkit Graveyard :: Notifications and Alerts, defect, P1)

72 Branch
Desktop
All
defect

Tracking

(firefox-esr68 unaffected, firefox71 unaffected, firefox72+ disabled, firefox73+ wontfix, firefox74+ verified, firefox75+ fixed)

RESOLVED FIXED
mozilla75
Tracking Status
firefox-esr68 --- unaffected
firefox71 --- unaffected
firefox72 + disabled
firefox73 + wontfix
firefox74 + verified
firefox75 + fixed

People

(Reporter: alice0775, Assigned: ntim)

References

(Regression)

Details

(Keywords: nightly-community, regression)

Attachments

(8 files)

Attached image screenshot (deleted) β€”

Reproducible: always
It is reproduced on [ja] localized build with default OS text size, and on [en-US] with 125% OS bigger text size.

Steps to reproduce:

  1. Open https://addons.mozilla.org/en-US/firefox/addon/duckduckgo-html/?src=search
  2. Click [Add to Firefox] button

Actual results:
"Add Search Engine" confirmation dialog button and description are overlapped each other

Expected results:
should not overlap

Regression window:
https://hg.mozilla.org/integration/autoland/pushloghtml?fromchange=cefb0e6f314aa176684e1fce4b71d649ca674c2d&tochange=70b2e0edb9a100ab95af8d7713f99dbc8ebd1ca0

Attached image screenshot ja build (deleted) β€”

[Tracking Requested - why for this release]: Visual layout regression, probably common to most dialogs.

Component: Search → Notifications and Alerts
Flags: needinfo?(ntim.bugs)
Product: Firefox → Toolkit

I can repro this on Linux with a regular en-US nightly too, fwiw, and without extra scaling (window.devicePixelRatio = 2, it's a hidpi screen).

OS: Windows 10 → All

Emilio mentioned that the dialog width changed between the first dialog opening and the second one on his side, presumably because the icon is loading. On Windows, the width is consistent across openings, probably because of this rule: https://searchfox.org/mozilla-central/source/toolkit/themes/windows/global/global.css#42-48

Regardless of the dialog width, I think this is related to the description spanning across multiple lines and an element miscalculating its height in that specific case. Maybe this can be solved similarly to bug 1602939 ?

Flags: needinfo?(ntim.bugs)

We're in the last day of 72 RC week so time is short if we want to address this.

Do you think this is release-blocking? Can we back out bug 1583925 safely or is that going to have other unwanted side effects?

Flags: needinfo?(standard8)
Flags: needinfo?(dao+bmo)

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

Do you think this is release-blocking?

Personally, I think this and bug 1606152 combined are a fairly poor experience for users that are going to hit it. I'm generally of the opinion that if there's bad layout, the user might think that we don't care so much, and also what else is wrong with the product and then they end up just going elsewhere.

That might need to be balanced with how many people are likely to hit this, it probably isn't a common route hit by a small percentage - though it might be more likely to happen users getting everything set up.

One thing to note is the code at issue is the common dialog code, which I assume drives a lot of our other pop-ups - so could affect more that haven't been picked up on. Unfortunately I don't know enough about the actual issue here to make that assessment.

Can we back out bug 1583925 safely or is that going to have other unwanted side effects?

Dao/Tim need to answer that.

Flags: needinfo?(standard8) → needinfo?(ntim.bugs)

(In reply to Mark Banner (:standard8) from comment #6)

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

Do you think this is release-blocking?

Personally, I think this and bug 1606152 combined are a fairly poor experience for users that are going to hit it. I'm generally of the opinion that if there's bad layout, the user might think that we don't care so much, and also what else is wrong with the product and then they end up just going elsewhere.

That might need to be balanced with how many people are likely to hit this, it probably isn't a common route hit by a small percentage - though it might be more likely to happen users getting everything set up.

One thing to note is the code at issue is the common dialog code, which I assume drives a lot of our other pop-ups - so could affect more that haven't been picked up on. Unfortunately I don't know enough about the actual issue here to make that assessment.

Not every dialog using commonDialog.xul seems affected by this (notably the ones I've originally tested), but yeah this is a poor experience and there may be other breakage we haven't caught.

Can we back out bug 1583925 safely or is that going to have other unwanted side effects?

Dao/Tim need to answer that.

Seems feasible for Firefox 72 (which seems like what you're asking here), but for Firefox 73, bug 1602939 would need to be backed out alongside it and bug 1585482 and bug 1601093 which landed in between would likely cause conflicts. Julien, could that be done for Firefox 72 for the time being? Thanks!

Flags: needinfo?(ntim.bugs) → needinfo?(jcristau)

OK if we're going to backout for a last minute RC respin, can we figure out what dialogs may be affected so QA can check we're not breaking something else?

Flags: needinfo?(jcristau)

Here are a lot of dialogs using the commonDialog mechanism:

Thanks Tim!
Does https://hg.mozilla.org/mozilla-central/rev/e14237c29b51 need to be backed out as well or is that OK to keep?

Flags: needinfo?(ntim.bugs)

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

Thanks Tim!
Does https://hg.mozilla.org/mozilla-central/rev/e14237c29b51 need to be backed out as well or is that OK to keep?

Oh, yeah, it would be a good idea to back this out too. Good catch, thanks!

Flags: needinfo?(ntim.bugs)
Flags: qe-verify+
Flags: needinfo?(dao+bmo)

Hi guys, while being unsuccessful in reproducing the issue on Nightly (2020-01-02) under windows 7 and 10, where the screen scale was already set to above 100% before opening the browser, I partially reproduced it on latest Nightly (2020-01-06) with the dialog button already open and then changing the scale from Windows settings. By doing this, the issue was visible over 125-150% scale, and by bringing it down to 100%, the dialog remained oversized.

See here the screencast. Please let me know if I could help with anything else.

Alice0775, can you still reproduce the issue with this try build: https://treeherder.mozilla.org/#/jobs?repo=try&revision=9b90dc1bb4e04f0ddc043d09645d35bb833ff06b ? Thanks :)

(In reply to Catalin Sasca, QA [:csasca] from comment #13)

See here the screencast. Please let me know if I could help with anything else.

Seems similar to the issue that's described here. Could you please check if it's fixed in Firefox 72 ? Thanks :)

Flags: needinfo?(catalin.sasca)
Flags: needinfo?(alice0775)

With STR comment #0,

  • Firefox 72.0 Windows10, the overlaps was fixed. However, bottom of [Add] [Cancel] buttons are cut off(similar to Bug 1600919).

  • The try build did not fix. I can still see the overlaps.

Flags: needinfo?(alice0775)

Tried on 72.0.1 and the cut off for [Add] and [Cancel] buttons was reproducible (as Alice said), and the oversize issue was still present too if lowering the scale percent as in the screencast.

Flags: needinfo?(catalin.sasca)

DΓ£o, can you own this for 73 while Tim is unavailable?

Flags: needinfo?(dao+bmo)
Priority: -- → P1

Hi,
I was able to reproduce this issue in Firefox Release 72.0.1 for Ubuntu 18.04.3 LTS, and in Firefox Release 72.0.1 and in the latest Nightly version 74.0a1 (2020-01-09) (64-bit) only could reproduce it when setting the screen scale to 175% only. Hope this information could be helpful.

Based on this I will mark firefox74 flag as affected.

Regards,
JerΓ³nimo.

Flags: needinfo?(dao+bmo) → needinfo?(ntim.bugs)

Brian, is this something you have time to look into ?

Flags: needinfo?(ntim.bugs) → needinfo?(bgrinstead)
QA Whiteboard: [qa-regression-triage]

Matt, could you help us find an owner for this bug? Thanks

Flags: needinfo?(MattN+bmo)

(In reply to Tim Nguyen :ntim from comment #19)

Brian, is this something you have time to look into ?

Not really. Maybe we should back out Bug 1583925 in the meantime? I'm not sure if this will interact with things that landed on top of it like Bug 1602939 & Bug 1605724, though.

Flags: needinfo?(bgrinstead)

FYI, dholbert is looking into a similar issue in bug 1610597 where the container height is wrongly calculated due to wrapping description text, so that might solve this bug too.

Maybe we should back out Bug 1583925 in the meantime?

This isn't particularly trivial to back out, but I'm open to this.

Backed out changeset f81ec1f37a25


Backed out changeset 234701139a2a


Backed out changeset e14237c29b51


Backed out changeset 70b2e0edb9a1

Assignee: nobody → ntim.bugs
Status: NEW → ASSIGNED

Comment on attachment 9124574 [details]
Bug 1606617 - Backed out 4 changesets for causing a regression.

Beta/Release Uplift Approval Request

  • User impact if declined: Attachment 9118322 [details].
  • Is this code covered by automated tests?: No
  • Has the fix been verified in Nightly?: No
  • Needs manual test from QE?: Yes
  • If yes, steps to reproduce: See comment 0
  • List of other uplifts needed: None
  • Risk to taking this patch: Low
  • Why is the change risky/not risky? (and alternatives if risky): Backout patch
  • String changes made/needed: none
Attachment #9124574 - Flags: approval-mozilla-beta?

Alice0775, do you know if there any similar dialogs affected by this type of bug (eg. involving wrapping text in description + a checkbox) ?

Mark, do you know any other way to trigger this popup ? The link from comment 0 seems down (AMO seems to have taken down all search engines not using the WE API).

Flags: needinfo?(standard8)
Flags: needinfo?(alice0775)

(In reply to Tim Nguyen :ntim from comment #25)

Alice0775, do you know if there any similar dialogs affected by this type of bug (eg. involving wrapping text in description + a checkbox) ?

No I do not know. I could not find any other similar dialog on Nightly74.0a1.

Flags: needinfo?(alice0775)
Attached file Test case part 2 (html file to click) (deleted) β€”

Here's a standalone test case that will trigger the dialog from Bugzilla. I've just used The Register's OpenSearch description for this as that's the first one I found.

Flags: needinfo?(standard8)

Comment on attachment 9124574 [details]
Bug 1606617 - Backed out 4 changesets for causing a regression.

It seems pretty scary taking this now during RC week when we'll have very limited time to look for fallout from this backout. I think we need to live with this bug in Fx73 :(

Attachment #9124574 - Flags: approval-mozilla-beta? → approval-mozilla-beta-
Attachment #9124574 - Flags: approval-mozilla-beta-
Flags: needinfo?(MattN+bmo)

Comment on attachment 9124574 [details]
Bug 1606617 - Backed out 4 changesets for causing a regression.

Beta/Release Uplift Approval Request

  • User impact if declined: Attachment 9118322 [details]
  • Is this code covered by automated tests?: No
  • Has the fix been verified in Nightly?: No
  • Needs manual test from QE?: Yes
  • If yes, steps to reproduce: See comment 0
  • List of other uplifts needed: None
  • Risk to taking this patch: Low
  • Why is the change risky/not risky? (and alternatives if risky): Backout patch
  • String changes made/needed:
Attachment #9124574 - Flags: approval-mozilla-beta?

Comment on attachment 9124574 [details]
Bug 1606617 - Backed out 4 changesets for causing a regression.

Backs out a number of commits to fix a UI regression. Approved for 74.0b2.

Attachment #9124574 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
QA Whiteboard: [qa-regression-triage] → [qa-triaged][qa-regression-triage]

I can still reproduce this issue with 74.0b2 with scale egal or larger then 150% (both with en and ja locale). See the screenshot on two different native resolution at 150% scale.
As an additional note I can see the same problem with the Firefox exit panel.

Flags: needinfo?(ntim.bugs)

From what I can see, the dialog contents are cut-off but not overlapping as this bug mentions. It's not ideal, but I think it's the state it was in before (given that it's a backout). Alice0775, do you know if the cut-off is already a pre-existing issue ?

Flags: needinfo?(ntim.bugs) → needinfo?(alice0775)
Attached image the cut-off is a pre-existing issue. (deleted) β€”

(In reply to Tim Nguyen :ntim from comment #34)

From what I can see, the dialog contents are cut-off but not overlapping as this bug mentions. It's not ideal, but I think it's the state it was in before (given that it's a backout). Alice0775, do you know if the cut-off is already a pre-existing issue ?

AFAIK, yes, the cut-off is a pre-existing issue.

Flags: needinfo?(alice0775)

I can no longer reproduce the overlapping problem, that is targeted here, with Fx 74.0b2 on Windows 10, macOS 10.15 and Ubuntu 18.04.

Ryan can you update the flag so this doesn't show up in the list for uplifts?

Flags: needinfo?(ryanvm)
Flags: needinfo?(ryanvm)
Attachment #9124574 - Flags: approval-mozilla-beta+

This is about to head to beta again. What are we going to do about it?

Flags: needinfo?(ntim.bugs)

Alice0775, could you please check if this build fixes the issue on your side ? Thanks :)

https://treeherder.mozilla.org/#/jobs?repo=try&revision=701d3e2bc3967523ec5875ed5964b39ae0418b52

Flags: needinfo?(ntim.bugs) → needinfo?(alice0775)

(In reply to Tim Nguyen :ntim from comment #40)

Alice0775, could you please check if this build fixes the issue on your side ? Thanks :)

https://treeherder.mozilla.org/#/jobs?repo=try&revision=701d3e2bc3967523ec5875ed5964b39ae0418b52

Yes, the overlap issue is fixed.
However, the button cut off issue is back.
(tested on Windows10 Japanese version and Text zoom 150%)

Flags: needinfo?(alice0775)

(In reply to Alice0775 White from comment #41)

Yes, the overlap issue is fixed.

Thanks for checking!

However, the button cut off issue is back.

Could you please check if this version fixes the cutoff: https://treeherder.mozilla.org/#/jobs?repo=try&revision=5f1468d0dc9c82e78139d7b7fdd9d40b30a4e35e ? Thanks again :)

Flags: needinfo?(alice0775)

(In reply to Tim Nguyen :ntim from comment #42)

(In reply to Alice0775 White from comment #41)

Yes, the overlap issue is fixed.

Thanks for checking!

However, the button cut off issue is back.

Could you please check if this version fixes the cutoff: https://treeherder.mozilla.org/#/jobs?repo=try&revision=5f1468d0dc9c82e78139d7b7fdd9d40b30a4e35e ? Thanks again :)

The try fixed both issues. I can no longer reproduce the overlap and the cut off.

Flags: needinfo?(alice0775)
Attachment #9130564 - Attachment description: Bug 1606617 - Add <div> wrapper around #dialogGrid. → Bug 1606617 - Add <box> wrapper around #dialogGrid.
Pushed by ntim.bugs@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/936b10b881e6
Add <box> wrapper around #dialogGrid. r=Gijs
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla75
Regressions: 1620137
Has Regression Range: --- → yes
Product: Toolkit → Toolkit Graveyard
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: