"Add Search Engine" confirmation dialog button and description are overlapped each other
Categories
(Toolkit Graveyard :: Notifications and Alerts, defect, P1)
Tracking
(firefox-esr68 unaffected, firefox71 unaffected, firefox72+ disabled, firefox73+ wontfix, firefox74+ verified, firefox75+ fixed)
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)
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:
- Open https://addons.mozilla.org/en-US/firefox/addon/duckduckgo-html/?src=search
- 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
Reporter | ||
Comment 1•5 years ago
|
||
Comment 2•5 years ago
|
||
[Tracking Requested - why for this release]: Visual layout regression, probably common to most dialogs.
Comment 3•5 years ago
|
||
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).
Assignee | ||
Comment 4•5 years ago
|
||
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 ?
Updated•5 years ago
|
Comment 5•5 years ago
|
||
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?
Comment 6•5 years ago
|
||
(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.
Assignee | ||
Comment 7•5 years ago
|
||
(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!
Comment 8•5 years ago
|
||
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?
Updated•5 years ago
|
Assignee | ||
Comment 9•5 years ago
|
||
Here are a lot of dialogs using the commonDialog mechanism:
- HTTP Auth: https://lgg.epfl.ch/teaching/DGP2019/Slides/html/01_Introduction.html
- Clicking "Remove" on a profile in about:profiles (you need to create a profile in case you don't have a second one)
- Master password prompt (setup a master password and then going to about:logins for instance)
- This bug: https://addons.mozilla.org/en-US/firefox/addon/duckduckgo-html/?src=search -> "Add to Firefox"
- about:preferences#privacy -> Select the "Never remember history" option
- The "You're about to close N tabs" prompt when closing multiple tabs
- Right click on an extension toolbar button and click "Remove extension"
- Unblock download prompt for blocked downloads
- Clicking "Sign out..." in about:preferences#sync while being logged into Sync
- ... there's quite a few other minor ones, but these are the main ones I can think of: https://searchfox.org/mozilla-central/search?q=Services.prompt&case=false®exp=false&path=
Updated•5 years ago
|
Comment 10•5 years ago
|
||
Thanks Tim!
Does https://hg.mozilla.org/mozilla-central/rev/e14237c29b51 need to be backed out as well or is that OK to keep?
Assignee | ||
Comment 11•5 years ago
|
||
(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!
Comment 12•5 years ago
|
||
Fixed for 72.0-build4 by backout:
https://hg.mozilla.org/releases/mozilla-release/rev/d2bd57dbd560
Updated•5 years ago
|
Updated•5 years ago
|
Comment 13•5 years ago
|
||
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.
Assignee | ||
Comment 14•5 years ago
|
||
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 :)
Reporter | ||
Comment 15•5 years ago
|
||
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.
Comment 16•5 years ago
|
||
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.
Comment 17•5 years ago
|
||
DΓ£o, can you own this for 73 while Tim is unavailable?
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.
Updated•5 years ago
|
Assignee | ||
Comment 19•5 years ago
|
||
Brian, is this something you have time to look into ?
Updated•5 years ago
|
Comment 20•5 years ago
|
||
Matt, could you help us find an owner for this bug? Thanks
Comment 21•5 years ago
|
||
(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.
Assignee | ||
Comment 22•5 years ago
|
||
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.
Assignee | ||
Comment 23•5 years ago
|
||
Backed out changeset f81ec1f37a25
Backed out changeset 234701139a2a
Backed out changeset e14237c29b51
Backed out changeset 70b2e0edb9a1
Updated•5 years ago
|
Assignee | ||
Comment 24•5 years ago
|
||
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
Assignee | ||
Comment 25•5 years ago
|
||
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).
Reporter | ||
Comment 26•5 years ago
|
||
(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.
Comment 27•5 years ago
|
||
Comment 28•5 years ago
|
||
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.
Comment 29•5 years ago
|
||
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 :(
Updated•5 years ago
|
Updated•5 years ago
|
Updated•5 years ago
|
Assignee | ||
Comment 30•5 years ago
|
||
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:
Comment 31•5 years ago
|
||
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.
Comment 32•5 years ago
|
||
bugherder uplift |
Updated•5 years ago
|
Comment 33•5 years ago
|
||
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.
Assignee | ||
Comment 34•5 years ago
|
||
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 ?
Reporter | ||
Comment 35•5 years ago
|
||
(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.
Comment 36•5 years ago
|
||
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.
Comment 37•5 years ago
|
||
Ryan can you update the flag so this doesn't show up in the list for uplifts?
Updated•5 years ago
|
Comment 38•5 years ago
|
||
This is about to head to beta again. What are we going to do about it?
Assignee | ||
Comment 39•5 years ago
|
||
Assignee | ||
Comment 40•5 years ago
|
||
Alice0775, could you please check if this build fixes the issue on your side ? Thanks :)
https://treeherder.mozilla.org/#/jobs?repo=try&revision=701d3e2bc3967523ec5875ed5964b39ae0418b52
Reporter | ||
Comment 41•5 years ago
|
||
(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%)
Assignee | ||
Comment 42•5 years ago
|
||
(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 :)
Reporter | ||
Comment 43•5 years ago
|
||
(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.
Updated•5 years ago
|
Comment 44•5 years ago
|
||
Pushed by ntim.bugs@gmail.com: https://hg.mozilla.org/integration/autoland/rev/936b10b881e6 Add <box> wrapper around #dialogGrid. r=Gijs
Comment 45•5 years ago
|
||
bugherder |
Updated•5 years ago
|
Updated•1 year ago
|
Description
•