Page MenuHomePhabricator

"Learn more" z-index is placed above the transparent-shield leading to a white flash during slide in animation
Closed, ResolvedPublic3 Story Points

Description

Note the related T206354

We have a z-index problem.

Description

When opening the main navigation menu, the dark overlay screen causes the fade next to the Learn more link to become visible as a white element on the screen. I'm seeing this on:

  • Android / Chrome, Firefox
  • iOS / Safari, Chrome, Firefox
  • MacOS / Safari, Chrome, Firefox

This only occurs during the animation and happens quite quickly (blink and you miss it!)

Documentation

video at 0:04s

Development notes

This is a z-index issue.
.transparent-shield, .navigation-drawer has a z-index of 0 whereas .ambox-learn-more has a z-index of 1 (intentionally so that the gradient appears above any text). The learn more link however should appears below the shield.

Luckily z-index's are managed in a single file: mobile.less/mobile.variables.less with a few naughty CSS rules not using variables :

  • resources/mobile.notifications.overlay/NotificationsOverlay.less
  • MinervaNeue/resources/skins.minerva.content.styles/templates/ambox.less
  • MinervaNeue/skinStyles/ext.echo.styles.special/SpecialNotificationsOverlay.less
@z-indexBase: 0;
@z-indexOverlay: 1;
@z-indexOverOverlay: 2;

To fix this we will need to rethink z-indexes and ensure learn more is the same or lower than the transparent shield.
Increasing the z-index of transparent-shield will require increasing the z-index of other things e.g. @z-indexOverOverlay and z-indexBase so be careful!

QA instructions

Environment: staging
Browser & device: mobile (iOS/Safari, Android/Chrome), desktop (MacOS/Safari, Windows/Chrome)
Skin: mobile/default, desktop/Minerva
Steps:

  • visit a page on staging with a page issue banner, e.g. https://reading-web-staging.wmflabs.org/wiki/Devolution (if you don't see a page issue banner with a blue "Learn more" link you'll need to find a different page on staging that has an issue)
  • tap the hamburger button to open the side nav
  • verify that the white rectangle behind the "Learn more" link does not become visible as the dark overlay appears on top of the content page content (reference the images above for an example of what to look out for)

Acceptance criteria / sign off steps

  • AC1: Check all z-index values are defined by variable by running this code search as assuring there are no hard-coded values like in z-index: 1 in the results.
  • AC2: The bug is fixed

QA Results

ACStatusDetails
AC1✅ PassedT214550#4970746
AC2✅ PassedT214550#4963044

Event Timeline

alexhollender triaged this task as Normal priority.Jan 24 2019, 4:16 AM
alexhollender created this task.
Aklapper renamed this task from Learn more white flash when opening navigation menu to "Learn more" white flash when opening navigation menu.Jan 24 2019, 11:23 AM
Jdlrobson added a subscriber: Jdlrobson.

I cannot reproduce this on production. Are you sure the problem is still present?

I cannot reproduce this on production. Are you sure the problem is still present?

I captured that video yesterday, and am still seeing it. What device/browser are you using?

Okay, I see it now, but only on Android - not desktop Chrome. Blink and you miss it!

Jdlrobson renamed this task from "Learn more" white flash when opening navigation menu to "Learn more" z-index is placed above the transparent-shield leading to a white flash during slide in animation.Jan 24 2019, 10:35 PM
Jdlrobson updated the task description. (Show Details)
Jdlrobson set the point value for this task to 3.Feb 5 2019, 5:48 PM
Jdrewniak added a comment.EditedFeb 7 2019, 11:58 PM

as far as I can tell, setting this line to z-index:0; (or @z-indexBase) seems to fix the issue. Here's a video with the fix.
https://drive.google.com/file/d/1uhFH9Dc4SXFPI3cTj50ZEf9XGx3-_noB/view?usp=sharing

Change 489112 had a related patch set uploaded (by Jdrewniak; owner: Jdrewniak):
[mediawiki/skins/MinervaNeue@master] Change z-index of page-issues read-more link

https://gerrit.wikimedia.org/r/489112

It'd be good to put this on staging to double check, but I think the simple fix works.

FYI, re: z-index - it's a common misconception that z-index is a global value, but it's actually relative to a stacking context, which can be created by parent elements that are positioned absolute, or even a changes in opacity.

It seems as though both the read-more and transparent-shield are in the same stacking context, but for example, if I change the opacity to 0.9 on the .ambox, a new stacking context is created below the transparent-shield, and that also fixes the problem, see video 😳

Jdlrobson added a subscriber: Jdrewniak.

This is now on staging.

@Jdrewniak the issue is fixed on mobile \o/

I noticed when testing on desktop that the "Learn more" link is still on top of the transparent-shield cloaked-element (in the video below you'll also notice that the color changes to a brighter blue). This isn't high priority to fix from a UX perspective, although I wanted to mention it because a) maybe it's something we want to fix from a technical perspective, b) if we don't fix it now I imagine we'll forget about it f o r e v e r

If you don't think it's worth fixing feel free to move this along to QA. I've added QA instructions.

Change 489112 merged by jenkins-bot:
[mediawiki/skins/MinervaNeue@master] Change z-index of page-issues read-more link

https://gerrit.wikimedia.org/r/489112

@alexhollender I'm not sure what changed but I'm unable to see the bug with the blue read-more text being clickable, either on staging or locally on master, so I'm handing this over to QA :)

Edtadros reassigned this task from Edtadros to alexhollender.EditedFeb 12 2019, 5:10 AM
Edtadros added a subscriber: Edtadros.

@Jdrewniak & @alexhollender I'm not sure if this is a feature or a bug, but the "Learn more" link appears right justified when I check it with a phone aspect ratio but left justified when I check it with a tablet or desktop. I am happy to document the test for tablets or desktop, but when I click the hamburger with a phone emulation, the link goes off screen to the right so I cannot verify it. Let me know how you'd like me to proceed.

Tablet and Desktop:

Phone:

@Edtadros yup that's a feature.

As you mention, it's hard to take a screenshot of this bug because the "read more" goes off the screen. If you're able to do a screen-recording though, I think that would adequately let us compare the results to the video posted in the description. (If you're using an iPhone, it's got the screen-recording functionality built-in now!)

@Jdrewniak weird and cool — confirming that the issue I mentioned in T214550#4939796 seems to have resolved itself.

#JediMindTricks

Test Result

Acceptance Criteria #2: The bug is fixed.

Status: ✅ PASS
OS: iOS 12.1.4
Browser: Safari

Test Artifact(s):

Tablet:

iPhone:

Edtadros updated the task description. (Show Details)Feb 19 2019, 12:57 AM
Edtadros reassigned this task from Edtadros to Jdrewniak.Feb 19 2019, 12:59 AM

@Jdrewniak I am not sure how to verify the z-order variables. I'm not sure I have access. You can walk me through it, or just provide me a test artifact of some sort....screen shot, code snippet, etc...

Change 491459 had a related patch set uploaded (by Jdrewniak; owner: Jdrewniak):
[mediawiki/skins/MinervaNeue@master] Remove hard-coded z-index value from page-issues read-more link

https://gerrit.wikimedia.org/r/491459

Hey @Edtadros You can do a code-search through all our repos using this tool: https://codesearch.wmflabs.org/search/?q=z-index%3A&i=nope&files=&repos=Skin:MinervaNeue

There was one value that wasn't using the variables, but it was a unique use-case so I left it as is, however it just occurred to me that I could still symbolize that one value (the computed value remains the same, so there's no functional change) so I just pushed a small patch that does that.

Jdrewniak updated the task description. (Show Details)Feb 19 2019, 12:40 PM

Change 491459 merged by jenkins-bot:
[mediawiki/skins/MinervaNeue@master] Remove hard-coded z-index value from page-issues read-more link

https://gerrit.wikimedia.org/r/491459

Jdlrobson reassigned this task from Jdrewniak to Edtadros.Feb 20 2019, 12:57 AM

" AC1: Check all z-index values are defined by variable by runn" should pass now. I feel this is outside the scope of @Edtadros and it might be better for a developer to do this as part of sign off.

I'm reassigning to Edward so he can make that decision but also run any further tests that might be necessary!

Status: ✅ PASS

Acceptance Criteria #1

Test Artifact(s):

Thanks for the link @Jdrewniak! Sometimes all I need is the right tool for the job.

ovasileva closed this task as Resolved.Feb 21 2019, 9:03 AM

Looks like we're done here. Thanks all!

ovasileva updated the task description. (Show Details)Feb 21 2019, 9:04 AM