Outlook.com search field is ugly - styling issues on search bar due to -webkit-appearance: menulist-textfield
Categories
(Core :: Widget, defect, P1)
Tracking
()
Tracking | Status | |
---|---|---|
firefox-esr60 | --- | unaffected |
firefox62 | --- | unaffected |
firefox63 | + | wontfix |
firefox64 | + | verified |
firefox65 | --- | verified |
People
(Reporter: cfogel, Assigned: jwatt)
References
(Blocks 1 open bug)
Details
(Keywords: regression)
Attachments
(6 files)
41.41 KB,
image/png
|
Details | |
528.64 KB,
image/png
|
Details | |
46 bytes,
text/x-phabricator-request
|
MatsPalmgren_bugz
:
review+
pascalc
:
approval-mozilla-beta-
|
Details | Review |
861 bytes,
patch
|
Details | Diff | Splinter Review | |
35.98 KB,
image/png
|
Details | |
46 bytes,
text/x-phabricator-request
|
jwatt
:
review+
RyanVM
:
approval-mozilla-beta+
|
Details | Review |
[Affected versions]: - 63.0, 63.0b14, 64.0a1 (2018-10-19) [Affected platforms]: - win10x64, Ubuntu 16.04, macOS10.14; [Steps to reproduce]: 1. Launch Firefox; 2. Login with a valid account; 3. Observe the Search bar; 4. Click inside the Search bar; [Expected result]: - content is properly displayed, nothing is truncated; [Actual result]: - search bar has an old_style border displayed; - bar appears to be truncated at the right edge(most visible on Ubuntu_OS); [Regression range]: - bug 1480073 appears to be at fault; [Additional notes]: - attached screen-shot with the issues; - 62.0.3 is not affected; - Ubuntu OS is the most affected by these changes
Updated•6 years ago
|
![]() |
Assignee | |
Updated•6 years ago
|
![]() |
Assignee | |
Comment 1•6 years ago
|
||
I don't know why we handle StyleAppearance::MenulistTextfield the same as StyleAppearance::Menulist. There is no UA style that seems to use the `menulist-textfield` value. It seems like it would be much better to handle it the same as StyleAppearance::Textfield.
![]() |
Assignee | |
Comment 2•6 years ago
|
||
I don't have Linux handy to test, but on Mac and Windows, Chrome displays 'menulist-textfield' exactly the same as 'textfield', except that the former doesn't have a border, whereas the latter has what looks like a 1px wide grey border. In terms of computed style, an <input type=search> has the exact same computed style for both `-webkit-appearance: textfield` and `-webkit-appearance: menulist-textfield` (other than the value of `-webkit-appearance` itself, of course). So I guess the border for 'textfield' is purely a "native" theming thing.
![]() |
Assignee | |
Comment 3•6 years ago
|
||
If someone with Linux handy could double check the differences on https://bug1368555.bmoattachments.org/attachment.cgi?id=8998017 with one column set to 'textfield' and the other set to 'menulist-textfield', that'd be helpful.
Comment 4•6 years ago
|
||
Like this? :)
![]() |
Assignee | |
Comment 5•6 years ago
|
||
Many thanks. :)
![]() |
Assignee | |
Updated•6 years ago
|
![]() |
Assignee | |
Comment 6•6 years ago
|
||
![]() |
Assignee | |
Comment 7•6 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=fefb0249b2fb4641d44e9f9570f5f4065718204a
![]() |
Assignee | |
Comment 8•6 years ago
|
||
I made a typo in the Linux changes, but as expected (since 'menulist-textfield' isn't used in our code/tests) nothing of interest seemed to fail on that Try run. Nevertheless, pushed again to get the Linux tests to run (and so we have builds to test with): https://treeherder.mozilla.org/#/jobs?repo=try&revision=430d9d523daa955ea1933d5a5c899855a5aa335c
![]() |
Assignee | |
Updated•6 years ago
|
![]() |
Assignee | |
Comment 9•6 years ago
|
||
Mac build: https://queue.taskcluster.net/v1/task/e9sldv0PTp6TeoFu_RZTkQ/runs/0/artifacts/public/build/target.dmg Win build: https://queue.taskcluster.net/v1/task/XpvqBeuhQSqBzKZE5b5Zrg/runs/0/artifacts/public/build/target.zip Linux build: https://queue.taskcluster.net/v1/task/BCKCuFt_QDWP5936ZdASKQ/runs/0/artifacts/public/build/target.tar.bz2
![]() |
Assignee | |
Updated•6 years ago
|
![]() |
Assignee | |
Updated•6 years ago
|
![]() |
Assignee | |
Comment 10•6 years ago
|
||
Heads up. We'll need to decide what to do here. The visual regression is crappy, especially on Windows and Mac. The patch I have up for review makes us take the same code paths for 'menulist-textfield' as we do for 'textfield', except for making it skip painting the (native) border when the control is not focused. So it should be very low risk. Probably lower risk than disabling the pref for the -webkit-appearance alias. We're really late in the beta cycle though. Your thoughts would be appreciated.
![]() |
Assignee | |
Comment 11•6 years ago
|
||
I've tested that both the Mac and Windows builds from comment 9 to confirm they fix this bug. I haven't been able to test the Linux build since I'm still being unsuccessful get a working Linux install set up in VMware Fussion since the macOS Mojave update. It's late so I'm giving up until the morning, but if anyone else can test that and see if it works that'd be really great.
Comment 12•6 years ago
|
||
I've tested your Linux build. It fixed the problem.
![]() |
Assignee | |
Comment 13•6 years ago
|
||
Awesome, thanks! One thing I should add to preempt the question - I didn't fix widget/gtk/gtk2drawing.c because GTK2 is no longer supported. (I filed bug 1500637 to get that code removed.)
Comment 14•6 years ago
|
||
FTR, the relevant CSS rule on outlook.com only has "-webkit-appearance: menulist-textfield" with no other *appearance declarations.
Comment 15•6 years ago
|
||
Widget seems like a more accurate component given the changes here.
Comment 16•6 years ago
|
||
(In reply to Mats Palmgren (:mats) from comment #12) > I've tested your Linux build. It fixed the problem. Sorry, it seems I was confused about the exact nature of the bug. Your Linux build has a border both when it's unfocused/focused, which isn't correct, IIUC. Here's a simpler testcase, BTW: data:text/html,<input style="-webkit-appearance:menulist-textfield">
Comment 17•6 years ago
|
||
So, if I apply your patch and then this patch on top of that, then it seems to work correctly. It paints a border when the <input> is focused, but not otherwise.
Comment 18•6 years ago
|
||
Sorry for the lag, I really wanted to apply this patch to test it on Linux, but I haven't have the time yet. Looks like mats has taken a look... If he's fine with the patch I'm fine with it as well. I'll try to look at this early tomorrow morning otherwise, though I'll be on a plane later. Sorry again for the lag.
Comment 19•6 years ago
|
||
Comment on attachment 9018785 [details] Bug 1500423. Make '-webkit-appearance: menulist-textfield' behave like Chrome. r?emilio Actually just took a look a bit out of guiltiness :) Mats' comments are really on point. Thanks Mats!
Comment 20•6 years ago
|
||
This is very likely to be too late for 63 but Pascal and Ritu will make a call!
![]() |
Assignee | |
Comment 21•6 years ago
|
||
(In reply to Emilio Cobos Álvarez (:emilio) from comment #19) > Actually just took a look a bit out of guiltiness :) You definitely should not be feeling guilty about that. It's super late on a Saturday evening, and you work very long hours anyway!
![]() |
Assignee | |
Comment 22•6 years ago
|
||
(In reply to Mats Palmgren (:mats) from comment #17) > Created attachment 9018853 [details] [diff] [review] > 1500423-linux-fix > > So, if I apply your patch and then this patch on top of that, > then it seems to work correctly. It paints a border when > the <input> is focused, but not otherwise. Ah, I didn't see the in bug comments until I'd updated the patch. I've addressed this, but by reverting my change to that section of the code. I've also pushed to Try to get a new Linux build to test this just in case anyone cares about that: https://treeherder.mozilla.org/#/jobs?repo=try&revision=e38ce9008e8fccc0c15f47b627453e657e07f016
![]() |
Assignee | |
Comment 23•6 years ago
|
||
For the Linux code, it looks like I should change moz_gtk_get_widget_border too. I'm making progress with VMware so hopefully I should be able to test myself (and build!) on Linux soon.
![]() |
Assignee | |
Comment 24•6 years ago
|
||
(In reply to Jonathan Watt [:jwatt] from comment #23) > For the Linux code, it looks like I should change moz_gtk_get_widget_border > too. New Try push and Linux build with that change: https://treeherder.mozilla.org/#/jobs?repo=try&revision=ff83b956db17fb92569763f7a66aeafb594bf929 https://queue.taskcluster.net/v1/task/AezMF8XWRliasPXbBBR5iQ/runs/0/artifacts/public/build/target.tar.bz2 My VMware setup still has problems but I can confirm that this change is both necessary and that that Linux build appears to behave correctly with it. I'll update the phab rev.
![]() |
Assignee | |
Comment 25•6 years ago
|
||
Phabricator is being a bit infuriating. I've marked all your requested changes as done, and uploaded multiple new revisions in an attempt to trigger it to re-request review. Still nothing. I'm request review here instead. *sigh*
![]() |
Assignee | |
Updated•6 years ago
|
![]() |
Assignee | |
Updated•6 years ago
|
Comment 26•6 years ago
|
||
Comment on attachment 9018785 [details] Bug 1500423. Make '-webkit-appearance: menulist-textfield' behave like Chrome. r?emilio r+ with a nit in the patch comments -- resolve as you see fit
Comment 27•6 years ago
|
||
Pushed by jwatt@jwatt.org: https://hg.mozilla.org/integration/mozilla-inbound/rev/bacc9594a048 Make '-webkit-appearance: menulist-textfield' behave like Chrome. r=mats
![]() |
Assignee | |
Comment 29•6 years ago
|
||
Comment on attachment 9018785 [details] Bug 1500423. Make '-webkit-appearance: menulist-textfield' behave like Chrome. r?emilio [Beta/Release Uplift Approval Request] Feature/Bug causing the regression: Bug 1368555 User impact if declined: Ugly looking search field at the top of outlook.com. See the attached screenshots. Definite risk of regressions on other sites. Is this code covered by automated tests?: No Has the fix been verified in Nightly?: Yes Needs manual test from QE?: Yes If yes, steps to reproduce: See comment 0 and comment 3. List of other uplifts needed: None Risk to taking this patch: Low Why is the change risky/not risky? (and alternatives if risky): The change makes us strictly take the same code paths for 'menulist-textfield' as we already do for 'textfield', with the exception of making it skip a native theming border painting call where appropriate. String changes made/needed: none
![]() |
Assignee | |
Comment 30•6 years ago
|
||
(In reply to Jonathan Watt [:jwatt] from comment #29) > Is this code covered by automated tests?: No Well, there are plenty of -moz-appearance tests in general, but the small difference between 'menulist-textfield' and 'textfield' is hard to test, so there's no specific tests for that difference. I have done a bunch of manual testing though. > Has the fix been verified in Nightly?: Yes It's been through a full Try push, but hasn't made it to our Nightly testers, strictly speaking. > Needs manual test from QE?: Yes Let's say yes, since it's worth having a second set of eyes on something that has even the slightest of chances of being uplifted this late in the game.
Comment 31•6 years ago
|
||
(In reply to Jonathan Watt [:jwatt] from comment #30) > Well, there are plenty of -moz-appearance tests in general, but the small > difference between 'menulist-textfield' and 'textfield' is hard to test, so > there's no specific tests for that difference. I have done a bunch of manual > testing though. It seems it should be possible to make a mochitest/reftest for this by comparing a focused 'menulist-textfield' to a focused 'textfield'. They should always render identically, right? (And a test that the unfocused controls renders differently.) Probably needs to be a mochitest though (unless we can rely on autofocus to work reliably in reftests?), i.e. adding it to test_reftests_with_caret.html which has the benefit of also testing that the caret appears.
Comment 32•6 years ago
|
||
Mike, Peter -- Heads up that this bug affects Firefox 63. It's due to our update of -webkit-appearance to address a number of outstanding webcompat issues. We know this bug affects Outlook.com, and I'm reaching out to MS to see what's possible to address this (if we can't or choose not to uplift this patch before 63 ships).
Updated•6 years ago
|
Comment 33•6 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/bacc9594a048
Reporter | ||
Comment 34•6 years ago
|
||
Verified the fix with the latest nightly build (64.0a1 (2018-10-21); the issues on Outlook appear to be fixed. Checking with the test case from comment 3, appears to have some issues on Windows OS. For the menulist-textfield option the backround is not displayed at all(see attachment). Is that expected as well, since the border is mentioned in comment 29?
![]() |
Assignee | |
Comment 35•6 years ago
|
||
Thanks for noticing that! I'll look into it now.
Comment 36•6 years ago
|
||
I see that [:jwatt] raised this on our coordination list and that folks from Microsoft are looking into it.
Updated•6 years ago
|
Updated•6 years ago
|
![]() |
Assignee | |
Comment 37•6 years ago
|
||
Taking this uplift today and respinning 63.0 RC would mean a delay in 63 launch. This bug as such doesn't seem like a blocker to me. I've added it to our tracking list of dot-release ride-alongs. If there is a 63.0.1, we'll do our best to include this fix. Thanks!
Comment 39•6 years ago
|
||
(In reply to Maire Reavy [:mreavy] Plz needinfo from comment #32) > Mike, Peter -- Heads up that this bug affects Firefox 63. It's due to our > update of -webkit-appearance to address a number of outstanding webcompat > issues. We know this bug affects Outlook.com, and I'm reaching out to MS to > see what's possible to address this (if we can't or choose not to uplift > this patch before 63 ships). Thanks Maire. Update here: on the mailing list, the relevant person who owns the stylesheet has been cc'd, as of yesterday.
![]() |
Assignee | |
Comment 40•6 years ago
|
||
The Windows changes have an issue with background painting anyway, which is visible when painting onto a differently colored background. I've been working on that today but encountered a bunch of headache on the way to getting a fix. That slightly hacky fix is now up at https://bugzilla.mozilla.org/attachment.cgi?id=9019065 . I requested review from jimm, but since we're not taking this for 63.0 he's redirected the review request to agashlin.
Comment 41•6 years ago
|
||
Comment on attachment 9018785 [details] Bug 1500423. Make '-webkit-appearance: menulist-textfield' behave like Chrome. r?emilio Updating uplift request as per comment #38
![]() |
Assignee | |
Comment 42•6 years ago
|
||
Try push with the follow-up Windows changes: https://treeherder.mozilla.org/#/jobs?repo=try&revision=d772acc964f04944edecac1d36279a2798405552 Windows build from that Try push for testing: https://queue.taskcluster.net/v1/task/Zr2MOClcTByS6e71bCeytw/runs/0/artifacts/public/build/target.zip
Comment 43•6 years ago
|
||
We received word last night that the Microsoft team made the fixes and pushed the changes to production.
![]() |
Assignee | |
Comment 44•6 years ago
|
||
Which is great. Nevertheless, the fact that this wasn't reported until just before release on a major site like outlook.com (and even then only by cfogel, rather than by a normal beta user) indicates that beta builds aren't getting enough end user testing. The chances of there being other major sites regressed by this that will be reported as Firefox 63 roles out is probably not insignificant. I think we should therefore still aim to be ready to take this in a point release if necessary.
Comment 45•6 years ago
|
||
Pushed by jwatt@jwatt.org: https://hg.mozilla.org/integration/mozilla-inbound/rev/c5f42f6a73c5 Paint the fill for menulist-textfield manually. r=agashlin
Comment 46•6 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/c5f42f6a73c5
Comment 47•6 years ago
|
||
Good point :jwatt - we are actively reaching out to partners about a number of topics in an effort to surface such issues more quickly. That doesn't directly answer your question, but it should help.
Comment 48•6 years ago
|
||
It'd be really nice to have at least a simple test for this... I think the test I suggested comment 31 would have caught the Windows regression from the first patch for example.
![]() |
Assignee | |
Comment 49•6 years ago
|
||
Yes, I should have said, I will be doing that.
![]() |
Assignee | |
Comment 50•6 years ago
|
||
I've spun off bug 1501838 to do that (rather than reopen this bug and create a bunch of additional noise for people that aren't interested in that part).
![]() |
Assignee | |
Comment 51•6 years ago
|
||
Comment on attachment 9019065 [details] Bug 1500423. Paint the fill for menulist-textfield manually. r?jmathies [Beta/Release Uplift Approval Request] Feature/Bug causing the regression: None User impact if declined: Is this code covered by automated tests?: Yes Has the fix been verified in Nightly?: Yes Needs manual test from QE?: Yes If yes, steps to reproduce: List of other uplifts needed: None Risk to taking this patch: Low Why is the change risky/not risky? (and alternatives if risky): String changes made/needed:
![]() |
Assignee | |
Comment 52•6 years ago
|
||
Hmm, that form didn't submit as intended. In summary, this is needed to fix a painting issue with the initial patch that is already on beta 64.
Updated•6 years ago
|
Comment 53•6 years ago
|
||
Comment on attachment 9019065 [details] Bug 1500423. Paint the fill for menulist-textfield manually. r?jmathies Follow-up fix for the patch which already landed on 64 prior to the Beta merge. Approved for 64.0b4.
Comment 54•6 years ago
|
||
bugherder uplift |
https://hg.mozilla.org/releases/mozilla-beta/rev/7869c932fb2b
Reporter | ||
Comment 55•6 years ago
|
||
Verified with the current nightly build 65.0a1 (2018-10-24). Issues from comment 34 are no longer encountered.
Reporter | ||
Comment 56•6 years ago
|
||
Verified with 64.0b4 as well on the mentioned OSs.
Comment 57•6 years ago
|
||
Jonathan, could you request uplift to release please? Thanks
![]() |
Assignee | |
Comment 58•6 years ago
|
||
Mike, what are your thoughts on comment 57? I haven't seen any follow-up bugs filed by your team for the list of sites using 'menulist-textfield' that Simon made up (and I didn't spot any significant issues myself). The patches here are not without risk, so if there are no other bad regressions then maybe we shouldn't take this for Release?
Comment 59•6 years ago
|
||
(In reply to Jonathan Watt [:jwatt] from comment #58) > Mike, what are your thoughts on comment 57? I haven't seen any follow-up > bugs filed by your team for the list of sites using 'menulist-textfield' > that Simon made up (and I didn't spot any significant issues myself). The > patches here are not without risk, so if there are no other bad regressions > then maybe we shouldn't take this for Release? Yeah, I'm about halfway through that list, and haven't seen anything bug-worthy yet. Since Outlook fixed this on their end, it's probably safer to let this bake on Beta.
![]() |
Assignee | |
Comment 60•6 years ago
|
||
Thanks, Mike. Pascal, back to you for your thoughts.
Comment 61•6 years ago
|
||
Thanks for your feedback, given that outlook.com is fixed and we don't have other webcompat problems reported so far that this patch would fix, let's not take it for 63.0.1.
Updated•6 years ago
|
Comment 62•6 years ago
|
||
I think treating menulist-textfield like 'none' is simpler than what was implemented here and likely also web-compatible.
From these pages (see comment at the end of the page how this was gathered):
https://gist.github.com/zcorpan/4684f8586b52638a75e661e8b75f27cb
I ran this in the console in Firefox Nightly:
[].slice.call(document.querySelectorAll('*')).filter(e=>getComputedStyle(e).WebkitAppearance === 'menulist-textfield').map(e=>e.style.WebkitAppearance='none')
These pages had one or more elements with -webkit-appearance: menulist-textfield:
https://imotors.com/
https://www.machinerypete.com/
Both of these have the same visual result with -webkit-appearance: none applied instead.
https://developer.microsoft.com/en-us/microsoft-edge/platform/usage/css/-moz-appearance/ shows menulist-textfield has 0.002% usage for the -moz-appearance property.
https://developer.microsoft.com/en-us/microsoft-edge/platform/usage/css/-webkit-appearance/ shows menulist-textfield is not among the top 20 values for -webkit-appearance, and is thus no more than 0.001%.
Comment 63•6 years ago
|
||
Spec PR per above comment: https://github.com/w3c/csswg-drafts/pull/3529
Description
•