Cristian Fogel [:cfogel] Reporter
	Description • 6 years ago

[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

	Jonathan Watt [:jwatt] 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.

	Jonathan Watt [:jwatt] 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.

	Jonathan Watt [:jwatt] 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.

	Emilio Cobos Álvarez (:emilio)
	Comment 4 • 6 years ago

Like this? :)

	Jonathan Watt [:jwatt] Assignee
	Comment 5 • 6 years ago

Many thanks. :)

	Jonathan Watt [:jwatt] Assignee
	Comment 7 • 6 years ago

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

	Jonathan Watt [:jwatt] 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

	Jonathan Watt [:jwatt] 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

	Jonathan Watt [:jwatt] 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.

	Jonathan Watt [:jwatt] 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.

	Mats Palmgren (inactive)
	Comment 12 • 6 years ago

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

	Jonathan Watt [:jwatt] 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.)

	Mats Palmgren (inactive)
	Comment 14 • 6 years ago

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

	Mats Palmgren (inactive)
	Comment 15 • 6 years ago

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

	Mats Palmgren (inactive)
	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">

	Mats Palmgren (inactive)
	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.

	Emilio Cobos Álvarez (:emilio)
	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.

	Emilio Cobos Álvarez (:emilio)
	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!

	Sylvestre Ledru [:Sylvestre]
	Comment 20 • 6 years ago

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

	Jonathan Watt [:jwatt] 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!

	Jonathan Watt [:jwatt] 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

	Jonathan Watt [:jwatt] 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.

	Jonathan Watt [:jwatt] 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.

	Jonathan Watt [:jwatt] 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*

	Mats Palmgren (inactive)
	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

	Pulsebot
	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

	Jonathan Watt [:jwatt] Assignee
	Comment 28 • 6 years ago

Simon, just a heads up on this change.

	Jonathan Watt [:jwatt] 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

	Jonathan Watt [:jwatt] 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.

	Mats Palmgren (inactive)
	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.

	Maire Reavy [:mreavy]
	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).

	Cristian Brindusan [:cbrindusan]
	Comment 33 • 6 years ago
bugherder

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

	Cristian Fogel [:cfogel] 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?

	Jonathan Watt [:jwatt] Assignee
	Comment 35 • 6 years ago

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

	Peter Saint-Andre [:stpeter]
	Comment 36 • 6 years ago

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

	Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)
	Comment 38 • 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!

	Mike Taylor [:miketaylr]
	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.

	Jonathan Watt [:jwatt] 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.

	Pascal Chevrel:pascalc (PTO until April 26)
	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

	Jonathan Watt [:jwatt] 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

	Peter Saint-Andre [:stpeter]
	Comment 43 • 6 years ago

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

	Jonathan Watt [:jwatt] 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.

	Pulsebot
	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

	Stefan Hindli [:stefan_hindli]
	Comment 46 • 6 years ago
bugherder

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

	Peter Saint-Andre [:stpeter]
	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.

	Mats Palmgren (inactive)
	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.

	Jonathan Watt [:jwatt] Assignee
	Comment 49 • 6 years ago

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

	Jonathan Watt [:jwatt] 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).

	Jonathan Watt [:jwatt] 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:

	Jonathan Watt [:jwatt] 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.

	Ryan VanderMeulen [:RyanVM]
	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.

	Ryan VanderMeulen [:RyanVM]
	Comment 54 • 6 years ago
bugherder uplift

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

	Cristian Fogel [:cfogel] 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.

	Cristian Fogel [:cfogel] Reporter
	Comment 56 • 6 years ago

Verified with 64.0b4 as well on the mentioned OSs.

	Pascal Chevrel:pascalc (PTO until April 26)
	Comment 57 • 6 years ago

Jonathan, could you request uplift to release please? Thanks

	Jonathan Watt [:jwatt] 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?

	Mike Taylor [:miketaylr]
	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.

	Jonathan Watt [:jwatt] Assignee
	Comment 60 • 6 years ago

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

	Pascal Chevrel:pascalc (PTO until April 26)
	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.