Page MenuHomePhabricator

"Notices" hitbox overlaps "alerts"
Closed, ResolvedPublic

Assigned To
Authored By
Pic-Sou
Oct 1 2020, 5:29 PM
Referenced Files
F33914553: Screen Recording 2020-11-14 at 4.36.08 PM.mov.gif
Nov 15 2020, 12:43 AM
F33914538: Screen Shot 2020-11-14 at 4.33.56 PM.png
Nov 15 2020, 12:43 AM
F33914557: Screen Shot 2020-11-14 at 4.38.41 PM.png
Nov 15 2020, 12:43 AM
F33914555: Screen Shot 2020-11-14 at 4.38.20 PM.png
Nov 15 2020, 12:43 AM
F33914561: Screen Shot 2020-11-14 at 4.40.21 PM.png
Nov 15 2020, 12:43 AM
F33914559: Screen Shot 2020-11-14 at 4.40.03 PM.png
Nov 15 2020, 12:43 AM
F33914533: Screen Shot 2020-11-14 at 4.30.45 PM.png
Nov 15 2020, 12:43 AM
F33914550: Screen Shot 2020-11-14 at 4.35.58 PM.png
Nov 15 2020, 12:43 AM
Tokens
"Burninate" token, awarded by JeanFred.

Description

Hi,

Since the last design change on frwikipedia (move of the Search bar), I noticed a bug in the behaviours of the "notices" and "alerts" buttons. Clicking on the "alerts" bell icon opens the "notices" box. The "alerts" box can still be opened using keyboard navigation. The "notices" button hitbox is far too wide (see image) and overlaps the "alerts" bell.

This bug happen whatever browser is used (checked with Firefox and Chrome), no matter the zoom level. It disappears if I use another account, but disabling all gadgets, all beta functionnalities and all scripts loaded in my .js and .css files didn’t fix it.

Several users are impacted: https://fr.wikipedia.org/wiki/Wikip%C3%A9dia:Le_Bistro/1_octobre_2020#Bug_de_l%E2%80%99ic%C3%B4ne_de_notification, but I do not know how to reproduce the bug.

Thanks in advance.

Capture du 2020-10-01 19:25:50.png (101×690 px, 17 KB)

QA

  • Check Vector skin
  • Check Minerva (mobile) skin
  • Check Monobook skin
  • Check Timeless skin

QA Results - Beta

QA Results - Prod

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes

Change 633243 had a related patch set uploaded (by Jdlrobson; owner: Jdlrobson):
[mediawiki/skins/Vector@master] Vertically align personal tools

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

Change 633244 had a related patch set uploaded (by Jdlrobson; owner: Jdlrobson):
[mediawiki/extensions/Echo@master] Drop text indent in modern Vector

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

Change 633243 merged by jenkins-bot:
[mediawiki/skins/Vector@master] Vertically align personal tools

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

Change 633244 merged by jenkins-bot:
[mediawiki/extensions/Echo@master] Drop text indent in modern Vector

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

This should be fixed. @ovasileva @MMiller_WMF we should QA this and potentially backport it, but it would be good to be clear which team should own doing these activities.

Change 634253 had a related patch set uploaded (by Jdlrobson; owner: Jdlrobson):
[mediawiki/skins/Vector@wmf/1.36.0-wmf.13] Vertically align personal tools

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

Change 634254 had a related patch set uploaded (by Jdlrobson; owner: Jdlrobson):
[mediawiki/extensions/Echo@wmf/1.36.0-wmf.13] Drop text indent in modern Vector

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

Change 634253 merged by jenkins-bot:
[mediawiki/skins/Vector@wmf/1.36.0-wmf.13] Vertically align personal tools

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

Change 634254 merged by jenkins-bot:
[mediawiki/extensions/Echo@wmf/1.36.0-wmf.13] Drop text indent in modern Vector

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

Mentioned in SAL (#wikimedia-operations) [2020-10-15T19:09:52Z] <catrope@deploy1001> Synchronized php-1.36.0-wmf.13/skins/Vector/: Vertically align personal tools (T264339) (duration: 01m 43s)

Mentioned in SAL (#wikimedia-operations) [2020-10-15T19:14:20Z] <catrope@deploy1001> Synchronized php-1.36.0-wmf.13/extensions/Echo/: Drop text indent in modern Vector (T264339) (duration: 01m 51s)

It looks like the click areas have been fixed. Thanks for all the hard work!

Evrifaessa added a subscriber: Evrifaessa.

This seems resolved. I can also confirm that the bug was fixed.

No, it hasn't been resolved. It has only shifted to the right. See the following image:

Screenshot_1.png (47×273 px, 2 KB)

They still overlap, and when I try to click on the first letter of [Mytalk], I end up clicking on the Notices icon.

Change 635019 had a related patch set uploaded (by Jdlrobson; owner: Jdlrobson):
[mediawiki/extensions/Echo@master] Item label can now use overflow hidden

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

Change 635020 had a related patch set uploaded (by Jdlrobson; owner: Jdlrobson):
[mediawiki/core@master] Wrap all menu items in a span in SkinMustache skins

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

I propose we start wrapping the text inside links with a span. As part of desktop improvements we're likely be seeing more icons, so I think this is a good strategic move and given the related regressions the current implementation of the Echo icon is too brittle. We could of course just apply the span wrapper to Echo if we wanted to be more conservative, but I would like to take the onus away from extensions of having to know about this implementation detail.

No, it hasn't been resolved. It has only shifted to the right. See the following image:

Screenshot_1.png (47×273 px, 2 KB)

They still overlap, and when I try to click on the first letter of [Mytalk], I end up clicking on the Notices icon.

Indeed the fix did not take into account languages where "Alerts" translates to a longer word. The correct fix was to restore text-align: left to accommodate the old text hiding hack:
https://gerrit.wikimedia.org/r/c/mediawiki/skins/Vector/+/585629/52/resources/skins.vector.styles/layout.less#95

The long-term solution however is to wrap the text in a span and hide it with position: absolute as I've implemented in T261171#6407969:
https://github.com/DemianX0/mediawiki-extensions-Echo/commit/888753f5442169a557a60e5b2ca959cc556144b5#diff-dd78c04f04087df48650282dbda56b61600df921903721f48beca0ce55616e5aR100
Which solution will break the client-side JS of Echo as that is mostly a redundant duplicate of the PHP code, necessitating some changes there too.

Change 635020 abandoned by Jdlrobson:
[mediawiki/core@master] Wrap all menu items in a span in SkinMustache skins

Reason:
I don't have time to investigate the implications of this so for now we'll just amend the Echo item itself https://gerrit.wikimedia.org/r/c/mediawiki/extensions/Echo/ /635019

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

Jdlrobson added a subscriber: Edtadros.

Needs review from growth team.

Change 635019 merged by jenkins-bot:
[mediawiki/extensions/Echo@master] Item label can now use overflow hidden

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

Change 635020 restored by Jdlrobson:
[mediawiki/core@master] Wrap all menu items in a span in SkinMustache skins

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

I think i must have had the core change checked out when i tested this. I'm not seeing the span wrapper.

Change 635907 had a related patch set uploaded (by Jdlrobson; owner: Jdlrobson):
[mediawiki/core@master] Individual links can request span wrapping if needed

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

Sorry clearly I need to give this more focus than I have been. I thought this was a trivial fix but this is dredging out some deeper problems with the code.
While the situation is improved its still not perfect. I'll take a look Monday unless somebody beats me to it.

I see 3 options which I'll need to explore.

  1. Wrap all menu links in span https://gerrit.wikimedia.org/r/c/635020
  2. Allow individual menu links e.g. Echo to wrap in a span https://gerrit.wikimedia.org/r/635907
  3. Revise the icon styles to use ::before for the icon https://gerrit.wikimedia.org/r/c/mediawiki/extensions/Echo/+/637000 Echo icons are overflow hidden [NEW]

It seems OOUI also changes the icon label when clicked to add even more complexity to this.

I've got Jan up to date and with a fresh pair of eyes he's going to take a look and see if he can suggest some better solutions than the ones I have proposed.

Change 637000 had a related patch set uploaded (by Jdlrobson; owner: Jdlrobson):
[mediawiki/extensions/Echo@master] Echo icons are overflow hidden

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

For the record: this is a bug that I've fixed early July (ref), even before it was created (ref).
text-align: right caused the issue, > li {text-align: left} was the obvious fix.

With the hidden (screen reader) text of the menu items wrapped in <span> the correct solution will be different:

.mw-echo-notifications-badge {
	#pt-notifications-alert &,
	#pt-notifications-notice & {
		...
		> span {
			position: absolute;
			top: -9999px;
		}
	}
}

Note: permission to use and copy this code is granted if and only if proper credit is given in a code comment.

Change 637509 had a related patch set uploaded (by Jdrewniak; owner: Jdrewniak):
[mediawiki/skins/Vector@master] Convert personal menu to use flexbox alignment.

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

Change 637528 had a related patch set uploaded (by Jdlrobson; owner: Jdlrobson):
[mediawiki/extensions/Echo@master] Restore old Echo styling through several reverts

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

Change 637000 abandoned by Jdlrobson:
[mediawiki/extensions/Echo@master] Echo icons are overflow hidden

Reason:

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

Change 635020 abandoned by Jdlrobson:
[mediawiki/core@master] Wrap all menu items in a span in SkinMustache skins

Reason:
Ideally text would be wrapped in spans, but this approach has implications on other menus which we'd rather avoid right now.

We've decided we don’t think have the time to drive through these larger architectural changes right now so have decided to restore the status quo in Echo and go for a Vector only fix.

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

Change 635907 abandoned by Jdlrobson:
[mediawiki/core@master] Individual links can request span wrapping if needed

Reason:
Ideally text would be wrapped in spans, but this approach has implications on other menus which we'd rather avoid right now.

We've decided we don’t think have the time to drive through these larger architectural changes right now so have decided to restore the status quo in Echo and go for a Vector only fix.

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

Change 637509 merged by jenkins-bot:
[mediawiki/skins/Vector@master] Convert personal menu to use flexbox alignment.

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

Change 637528 merged by jenkins-bot:
[mediawiki/extensions/Echo@master] Restore old Echo styling through several reverts

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

Jdlrobson moved this task from Code Review to QA on the Web-Team-Backlog (Kanbanana-FY-2020-21) board.
Jdlrobson added a subscriber: Jdrewniak.

Ideally text would be wrapped in spans, but this approach was not possible without a core change to allow Echo to wrap its items in spans (either https://gerrit.wikimedia.org/r/c/635020 or https://gerrit.wikimedia.org/r/c/635907) and that has implications on other menus which we'd rather avoid right now.

We've decided we don’t have the time to drive through these larger architectural changes right now so have decided to restore the status quo in Echo and go for a Vector only fix.

Moving to QA to confirm that Timeless and Vector are restored to normal.
Note existing problems with CologneBlue (pointer not connected to dialog), Modern (rendered in a squished space) and Monobook (jumps a few pixels when clicked) remain but those were issues before and unrelated and uneffected by these changes.

Test Result - Beta

Status: ✅ PASS
Environment: beta
OS: macOS Catalina
Browser: Chrome
Device: MBP
Emulated Device: NA

Test Artifact(s):

QA Steps

Verify the hitbox for Notices and Alerts do not overlap, the notices link opens notices and alerts opens alerts.

✅ AC1: Vector Legacy

Screen Shot 2020-11-02 at 6.19.22 PM.png (648×1 px, 231 KB)
Screen Shot 2020-11-02 at 6.19.42 PM.png (648×1 px, 209 KB)

✅ AC2: Vector New

Screen Shot 2020-11-02 at 6.21.01 PM.png (648×1 px, 268 KB)
Screen Shot 2020-11-02 at 6.21.15 PM.png (648×1 px, 237 KB)

✅ AC3: MinervaNeue Mobile (Notices only)

Screen Shot 2020-11-02 at 6.22.59 PM.png (648×1 px, 134 KB)
Screen Recording 2020-11-02 at 6.23.10 PM.mov.gif (648×1 px, 535 KB)

✅ AC4: Modern

Screen Shot 2020-11-02 at 6.31.35 PM.png (648×1 px, 170 KB)
Screen Shot 2020-11-02 at 6.32.01 PM.png (648×1 px, 154 KB)

✅ AC5: Monobook

Screen Shot 2020-11-02 at 6.32.45 PM.png (648×1 px, 260 KB)
Screen Shot 2020-11-02 at 6.33.01 PM.png (648×1 px, 243 KB)

✅ AC6: Timeless

Screen Shot 2020-11-02 at 6.33.32 PM.png (648×1 px, 229 KB)
Screen Shot 2020-11-02 at 6.33.45 PM.png (648×1 px, 208 KB)
Edtadros updated the task description. (Show Details)

Test Result - Prod

Status: ✅ PASS
Environment: enwiki
OS: macOS Catalina
Browser: Chrome
Device: MBP
Emulated Device: NA

Test Artifact(s):

QA Steps

Verify the hitbox for Notices and Alerts do not overlap, the notices link opens notices and alerts opens alerts.

✅ AC1: Vector Legacy

Screen Shot 2020-11-14 at 4.30.22 PM.png (707×1 px, 219 KB)
Screen Shot 2020-11-14 at 4.30.45 PM.png (707×1 px, 219 KB)

✅ AC2: Vector New

Screen Shot 2020-11-14 at 4.33.35 PM.png (707×1 px, 216 KB)
Screen Shot 2020-11-14 at 4.33.56 PM.png (707×1 px, 217 KB)

✅ AC3: MinervaNeue Mobile (Notices only)

Screen Shot 2020-11-14 at 4.35.58 PM.png (707×1 px, 133 KB)
Screen Recording 2020-11-14 at 4.36.08 PM.mov.gif (706×1 px, 340 KB)

✅ AC4: Modern

Screen Shot 2020-11-14 at 4.38.20 PM.png (707×1 px, 146 KB)
Screen Shot 2020-11-14 at 4.38.41 PM.png (707×1 px, 148 KB)

✅ AC5: Monobook

Screen Shot 2020-11-14 at 4.40.03 PM.png (707×1 px, 224 KB)
Screen Shot 2020-11-14 at 4.40.21 PM.png (707×1 px, 225 KB)

✅ AC6: Timeless

Screen Shot 2020-11-14 at 4.41.22 PM.png (707×1 px, 203 KB)
Screen Shot 2020-11-14 at 4.42.45 PM.png (707×1 px, 203 KB)