1393564 - Cannot click onboarding icon in old new tab

Status: VERIFIED FIXED

(Firefox :: New Tab Page, defect)

Description

[Oriol Brufau [:Oriol]]

1. Set browser.newtabpage.activity-stream.enabled=false
2. Open a new tab
3. There is this onboarding icon at the top-left edge. Try to click it.

Result: the cursor does not become a pointer when hovered and clicking does nothing.

https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=c4bd4820ca65c3f8ec85c371d018c8e45c5d614e&tochange=0f1ca58ad88e1fe575a297032d2db567b0b66ee3

Previously the icon was appended at the end of the body, now it's at the beginning, so it appears behind following elements. Move it back to the end or increase its z-index.

Comment 1

[Yura Zenevich [:yzen] (please use "needinfo")]

Attachment: 1393564 patch

Comment 2

[Oriol Brufau [:Oriol]]

Comment on attachment 8901824 [details] [diff] [review]
1393564 patch

Review of attachment 8901824 [details] [diff] [review]:
-----------------------------------------------------------------

::: browser/extensions/onboarding/content/onboarding.css
@@ +31,5 @@
>    offset-inline-start: 34px;
>    border: none;
>    /* Set to none so no grey contrast background in the high-contrast mode */
>    background: none;
> +  z-index: 20998; /* We want this always under #onboarding-overlay */

Why this absurdly high number? `z-index: 2` should work (or `z-index: 1` if `#newtab-margin-undo-container` was properly hidden).
Anyways, the icon is still not clickable at the top because of some negative margin.
You should remove `#onboarding-overlay-button::after { margin-top: -5px; }`, add `#onboarding-overlay-button-icon { margin-top: 5px }`, and change `#onboarding-overlay-button { top: 34px; } to `top: 29px`

Comment 3

[Yura Zenevich [:yzen] (please use "needinfo")]

(In reply to Oriol Brufau [:Oriol] from comment #2)
> Comment on attachment 8901824 [details] [diff] [review]
> 1393564 patch
> 
> Review of attachment 8901824 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: browser/extensions/onboarding/content/onboarding.css
> @@ +31,5 @@
> >    offset-inline-start: 34px;
> >    border: none;
> >    /* Set to none so no grey contrast background in the high-contrast mode */
> >    background: none;
> > +  z-index: 20998; /* We want this always under #onboarding-overlay */
> 
> Why this absurdly high number? `z-index: 2` should work (or `z-index: 1` if
> `#newtab-margin-undo-container` was properly hidden).

I'm merely using the same values that are already used for onboarding (e.g. notification bar)

Comment 4

[Yura Zenevich [:yzen] (please use "needinfo")]

(In reply to Oriol Brufau [:Oriol] from comment #2)
> Comment on attachment 8901824 [details] [diff] [review]
> 1393564 patch
> 
> Review of attachment 8901824 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: browser/extensions/onboarding/content/onboarding.css
> @@ +31,5 @@
> >    offset-inline-start: 34px;
> >    border: none;
> >    /* Set to none so no grey contrast background in the high-contrast mode */
> >    background: none;
> > +  z-index: 20998; /* We want this always under #onboarding-overlay */
> 
> Why this absurdly high number? `z-index: 2` should work (or `z-index: 1` if
> `#newtab-margin-undo-container` was properly hidden).
> Anyways, the icon is still not clickable at the top because of some negative
> margin.
> You should remove `#onboarding-overlay-button::after { margin-top: -5px; }`,
> add `#onboarding-overlay-button-icon { margin-top: 5px }`, and change
> `#onboarding-overlay-button { top: 34px; } to `top: 29px`

just ni'ing Fred, as I don't want to mangle with those values without extension peers.

Comment 5

[[e-mail]]

z-index:2 would work but we have to make sure the z-index is higher than activity stream's new tab settings icon(at top right), or the overlay will show below the AS's settings icon.

Comment 6

[[e-mail]]

Comment on attachment 8901824 [details] [diff] [review]
1393564 patch

Since this case is about the top left icon, we don't have to pick the high number for z-index. In Bug 1392467 I picked `z-index: 10;` which will solve this issue as well.

I'd leave the z-index change in this bug (z-index: 10) since its test-covered and can uplift to v56beta separately.

Comment 7

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/7efac96862f9
making onboarding overlay button z-index consistent with other elements (notification and dialog). r=gasolin

Comment 8

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

This and bug 1392468 both added a "z-index:10;" to #onboarding-overlay-button, just with different comments, so I just included both comments when merging them together. Feel free to work out if one/both of them should be dropped and do that.

Comment 9

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

https://hg.mozilla.org/mozilla-central/rev/7efac96862f9

Comment 10

[Yura Zenevich [:yzen] (please use "needinfo")]

(In reply to Wes Kocher (:KWierso) from comment #8)
> This and bug 1392468 both added a "z-index:10;" to
> #onboarding-overlay-button, just with different comments, so I just included
> both comments when merging them together. Feel free to work out if one/both
> of them should be dropped and do that.

I think this is not critical so lets leave the comments as is.

Comment 11

[Cristian Comorasu, QA [:ccomorasu], Releae Desktop QA - Please contact [e-mail]]

I can confirm this issue is fixed.
I verified using Fx 57.0a1 (2017-09-01) and Fx 56.0b8 on Windows 10 x64, mac OS X 10.12.6 and Ubuntu 14.04 LTS.

Cheers!

Comment 12

[Ryan VanderMeulen [:RyanVM]]

AFAIK, 56 was never affected in the first place?

Comment 13

[Cristian Comorasu, QA [:ccomorasu], Releae Desktop QA - Please contact [e-mail]]

Sorry for the long pause as I was on personal time off.
The tracking flags were not set, so I decided to verify the issue on beta as well.