1500423 - Outlook.com search field is ugly - styling issues on search bar due to -webkit-appearance: menulist-textfield

Status: VERIFIED FIXED

(Core :: Widget, defect, P1)

Description

[Cristian Fogel [:cfogel]]

Attachment: outlook_boxSearchStyling.png

[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

Comment 1

[Jonathan Watt [:jwatt]]

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.

Comment 2

[Jonathan Watt [:jwatt]]

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.

Comment 3

[Jonathan Watt [:jwatt]]

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

[Emilio Cobos Álvarez [:emilio]]

Attachment: Screenshot from 2018-10-19 16-52-31.png

Like this? :)

Comment 5

[Jonathan Watt [:jwatt]]

Many thanks. :)

Comment 6

[Jonathan Watt [:jwatt]]

Attachment: Bug 1500423. Make '-webkit-appearance: menulist-textfield' behave like Chrome. r?emilio

Comment 7

[Jonathan Watt [:jwatt]]

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

Comment 8

[Jonathan Watt [:jwatt]]

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

Comment 9

[Jonathan Watt [:jwatt]]

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

Comment 10

[Jonathan Watt [:jwatt]]

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.

Comment 11

[Jonathan Watt [:jwatt]]

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

[Mats Palmgren (inactive)]

I've tested your Linux build.  It fixed the problem.

Comment 13

[Jonathan Watt [:jwatt]]

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

[Mats Palmgren (inactive)]

FTR, the relevant CSS rule on outlook.com only has "-webkit-appearance: menulist-textfield"
with no other *appearance declarations.

Comment 15

[Mats Palmgren (inactive)]

Widget seems like a more accurate component given the changes here.

Comment 16

[Mats Palmgren (inactive)]

(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

[Mats Palmgren (inactive)]

Attachment: 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.

Comment 18

[Emilio Cobos Álvarez [:emilio]]

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

[Emilio Cobos Álvarez [:emilio]]

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

[Sylvestre Ledru [:Sylvestre]]

This is very likely to be too late for 63 but Pascal and Ritu will make a call!

Comment 21

[Jonathan Watt [:jwatt]]

(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!

Comment 22

[Jonathan Watt [:jwatt]]

(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

Comment 23

[Jonathan Watt [:jwatt]]

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.

Comment 24

[Jonathan Watt [:jwatt]]

(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.

Comment 25

[Jonathan Watt [:jwatt]]

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*

Comment 26

[Mats Palmgren (inactive)]

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

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/bacc9594a048
Make '-webkit-appearance: menulist-textfield' behave like Chrome. r=mats

Comment 28

[Jonathan Watt [:jwatt]]

Simon, just a heads up on this change.

Comment 29

[Jonathan Watt [:jwatt]]

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

Comment 30

[Jonathan Watt [:jwatt]]

(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

[Mats Palmgren (inactive)]

(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

[Maire Reavy [:mreavy]]

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).

Comment 33

[Cristian Brindusan [:cbrindusan]]

https://hg.mozilla.org/mozilla-central/rev/bacc9594a048

Comment 34

[Cristian Fogel [:cfogel]]

Attachment: textfield_menulistTextfield.png

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?

Comment 35

[Jonathan Watt [:jwatt]]

Thanks for noticing that! I'll look into it now.

Comment 36

[Peter Saint-Andre [:stpeter]]

I see that [:jwatt] raised this on our coordination list and that folks from Microsoft are looking into it.

Comment 37

[Jonathan Watt [:jwatt]]

Attachment: Bug 1500423. Paint the fill for menulist-textfield manually. r?jmathies

Comment 38

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

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

[Mike Taylor [:miketaylr]]

(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.

Comment 40

[Jonathan Watt [:jwatt]]

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

[Pascal Chevrel:pascalc]

Comment on attachment 9018785 [details]
Bug 1500423. Make '-webkit-appearance: menulist-textfield' behave like Chrome. r?emilio

Updating uplift request as per comment #38

Comment 42

[Jonathan Watt [:jwatt]]

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

[Peter Saint-Andre [:stpeter]]

We received word last night that the Microsoft team made the fixes and pushed the changes to production.

Comment 44

[Jonathan Watt [:jwatt]]

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

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/c5f42f6a73c5
Paint the fill for menulist-textfield manually. r=agashlin

Comment 46

[Stefan Hindli [:stefan_hindli]]

https://hg.mozilla.org/mozilla-central/rev/c5f42f6a73c5

Comment 47

[Peter Saint-Andre [:stpeter]]

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

[Mats Palmgren (inactive)]

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.

Comment 49

[Jonathan Watt [:jwatt]]

Yes, I should have said, I will be doing that.

Comment 50

[Jonathan Watt [:jwatt]]

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).

Comment 51

[Jonathan Watt [:jwatt]]

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:

Comment 52

[Jonathan Watt [:jwatt]]

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.

Comment 53

[Ryan VanderMeulen [:RyanVM]]

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

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/7869c932fb2b

Comment 55

[Cristian Fogel [:cfogel]]

Verified with the current nightly build 65.0a1 (2018-10-24).
Issues from comment 34 are no longer encountered.

Comment 56

[Cristian Fogel [:cfogel]]

Verified with 64.0b4 as well on the mentioned OSs.

Comment 57

[Pascal Chevrel:pascalc]

Jonathan, could you request uplift to release please? Thanks

Comment 58

[Jonathan Watt [:jwatt]]

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

[Mike Taylor [:miketaylr]]

(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.

Comment 60

[Jonathan Watt [:jwatt]]

Thanks, Mike. Pascal, back to you for your thoughts.

Comment 61

[Pascal Chevrel:pascalc]

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.

Comment 62

[Simon Pieters [:zcorpan]]

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

[Simon Pieters [:zcorpan]]

Spec PR per above comment: https://github.com/w3c/csswg-drafts/pull/3529