1403733 - “Activate now” button does not take full width in CtP doorhanger in Private Browsing

Status: VERIFIED FIXED

(Firefox :: Site Identity, defect, P1)

Description

[Théo Chevalier]

Attachment: Screenshot

In Private Browsing only, when opening the click-to-play doorhanger in Private Browsing, the “Activate now” button is using only half the panel (see attached screenshot).

STR:

- Open a new Private Window
- Open a page with Flash content (e.g. http://get.adobe.com/fr/flashplayer/about/)
- Click the plugin icon in the address bar

I can reproduce with a fresh profile on 57.0b1 and can’t reproduce on 55.0.3.
56.0 is probably unaffected, but I didn’t test.

Comment 1

[Johann Hofmann [:johannh]]

Doug, do you think this is a regression from bug 1392979?

Comment 2

[Alex Thayer [:alexical] (she/her)]

Yeah, I think so. Taking a look.

Comment 3

[Alex Thayer [:alexical] (she/her)]

Attachment: Bug 1403733 - Fix single button styling on Flash doorhanger

The "default" attribute in this case controls whether the
width gets set to 50%. An alternative to this might be setting an
extra class on the buttons to indicate that they aren't alone, which
we could look at in a stylesheet and avoid setting the width to 50%,
but this seems to be reasonably intuitive and a smaller change.

Review commit: https://reviewboard.mozilla.org/r/188114/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/188114/

Comment 4

[:Felipe Gomes (needinfo for replies!)]

Hm this is the corresponding CSS, right?
http://searchfox.org/mozilla-central/rev/31606bbabc50b08895d843b9f5f3da938ccdfbbf/toolkit/themes/shared/popupnotification.inc.css#81

The [default] here is used to set the 50% width, but it's used in other lines too for coloring etc. So I think it's not correct to do this.

I wish we could use some selector like this:
http://searchfox.org/mozilla-central/source/toolkit/themes/osx/global/notification.css#20

but the ~ implies the second element must come after the first one, so it would work for one case but not the other.

So I believe setting an extra class as you said might be best. (Note: it seems way more common to use an attribute rather than a class, so we should probably do that)

Comment 5

[:Felipe Gomes (needinfo for replies!)]

Comment on attachment 8917611 [details]
Bug 1403733 - Fix single button styling on Flash doorhanger

https://reviewboard.mozilla.org/r/188114/#review194160

Comment 6

[Alex Thayer [:alexical] (she/her)]

Comment on attachment 8917611 [details]
Bug 1403733 - Fix single button styling on Flash doorhanger

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/188114/diff/1-2/

Comment 7

[:Felipe Gomes (needinfo for replies!)]

Comment on attachment 8917611 [details]
Bug 1403733 - Fix single button styling on Flash doorhanger

https://reviewboard.mozilla.org/r/188114/#review195480

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/ef42d6855193
Fix single button styling on Flash doorhanger r=Felipe

Comment 9

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

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

Comment 10

[Sylvestre Ledru [:Sylvestre]]

I think we can let that ride the train, right?

Comment 11

[Alex Thayer [:alexical] (she/her)]

I'm fine with it riding the trains. I just got the patch ready in case Bug 1398972 isn't ready by 58 (since that will supersede this), but I think it will be. But, if anyone thinks this regression warrants an uplift, the patch isn't very risky.

Comment 12

[:Felipe Gomes (needinfo for replies!)]

Oh this is a regression from a bug that landed in 57, and the fix is fairly simple. I think we should uplift it.

Comment 13

[Alex Thayer [:alexical] (she/her)]

Comment on attachment 8917611 [details]
Bug 1403733 - Fix single button styling on Flash doorhanger

Approval Request Comment
[Feature/Bug causing the regression]: Bug 1392979
[User impact if declined]: Users will see bad styling on the Flash CTP doorhanger in private browsing windows.
[Is this code covered by automated tests?]: No.
[Has the fix been verified in Nightly?]: I have tested on latest Nightly.
[Needs manual test from QE? If yes, steps to reproduce]: No.
[List of other uplifts needed for the feature/fix]: None.
[Is the change risky?]: No.
[Why is the change risky/not risky?]: It only affects button styling.
[String changes made/needed]: None.

Comment 14

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

Comment on attachment 8917611 [details]
Bug 1403733 - Fix single button styling on Flash doorhanger

This is a minor issue, wontfix for 57.

Comment 15

[roxleitan]

Reproduced the issue on Nightly (2017-09-27).
Verified fixed using the latest Nightly 58.0a1 (2017-11-06) on Windows 10 x64, Mac OS X 10.12 and Ubuntu 16.04 x64.

Comment 16

[Oriol Brufau [:Oriol]]

Frankly the doorhanger looks broken in release. So bad this wasn't uplifted.