Closed Bug 1393564 Opened 4 years ago Closed 4 years ago

Cannot click onboarding icon in old new tab

Categories

(Firefox :: New Tab Page, defect)

defect
Not set
normal

Tracking

()

VERIFIED FIXED
Firefox 57
Tracking Status
firefox-esr52 --- unaffected
firefox55 --- unaffected
firefox56 --- verified
firefox57 --- verified

People

(Reporter: Oriol, Assigned: yzen)

References

Details

(Keywords: regression, Whiteboard: [photon-onboarding])

Attachments

(1 file)

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.
Attached patch 1393564 patchSplinter Review
Attachment #8901824 - Flags: review?(dtownsend)
Whiteboard: [photon-onboarding]
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`
(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)
(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.
Flags: needinfo?(gasolin)
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.
Flags: needinfo?(gasolin)
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.
Attachment #8901824 - Flags: review?(dtownsend) → review+
Blocks: 1390007
Assignee: nobody → yzenevich
Status: NEW → ASSIGNED
Pushed by yura.zenevich@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/7efac96862f9
making onboarding overlay button z-index consistent with other elements (notification and dialog). r=gasolin
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.
Flags: needinfo?(yzenevich)
https://hg.mozilla.org/mozilla-central/rev/7efac96862f9
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 57
(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.
Flags: needinfo?(yzenevich)
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!
Status: RESOLVED → VERIFIED
AFAIK, 56 was never affected in the first place?
Flags: needinfo?(cristian.comorasu)
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.
Flags: needinfo?(cristian.comorasu)
You need to log in before you can comment on or make changes to this bug.