1733580 - PiP toggle glitch in High Contrast Mode

Status: VERIFIED FIXED

(Toolkit :: Video/Audio Controls, defect, P3)

Description

[Alin Ilea, Desktop QA]

Attachment: 2021-10-01_15h29_17.png

Tested with:

Release 92
Beta 93
Nightly 94.0a1 (2021-10-01)

Tested on:

Windows 7
Windows 10

Preconditions:

Have the High Contrast enabled (black/white)
Make sure you are on a Firefox profile where PiP was toggled before.

Steps:

1. Launch Firefox and go to youtube.com.
2. Play a random video.
3. Hover over the video and check out the PiP toggle.

Actual result:

There is a small line displayed above the PiP toogle.

Expected result:

The PiP toggle icon should be displayed accordingly without any glitch.

Comment 1

[:Gijs (he/him)]

This is a result of the PiP markup using button as a container for its icon: https://searchfox.org/mozilla-central/rev/2c4b830b924f42283632b70f39a60fd36433dd4d/toolkit/content/widgets/videocontrols.js#2792-2804 . Note that all the contents are absolutely/relatively positioned, so the button itself has no size - except...

The button has horizontal and vertical padding, and that means we end up painting a rectangle that is 2x8px in HCM, because the background: none and color: inherit gets overwritten with the HCM background and foreground.

To fix this, I see a few different possibilities:

• set padding to 0, and ensure that the positioning of the icon doesn't shift before/after (which, at least if I do it in the inspector, it does, so it'd need some additional tweaking)
• find a way to set the background/foreground that doesn't result in us painting when in HCM;
• use a <div> with role=button instead of a "real" button (and ensure we don't lose any keyboard accessibility that we have today).

Micah or Molly, perhaps this is something the MSU folks could tackle as part of the HCM work?

Comment 2

[Eitan Isaacson [:eeejay]]

Marking as access-s4 since this is a minor defect that doesn't make the feature more difficult to use, or pose an additional accessibility challenge.

Comment 3

[Molly Howell (she/her) (no longer active)]

(In reply to :Gijs (he/him) from comment #1)

Micah or Molly, perhaps this is something the MSU folks could tackle as part of the HCM work?

I agree.

Comment 4

[Noah Pesta]

Attachment: Bug 1733580 - Fixed glitch on the PiP toggle icon in HCM. r=mtigley

Comment 5

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/dd40d53329ea
Fixed glitch on the PiP toggle icon in HCM. r=mtigley

Comment 6

[Marian-Vasile Laza]

Backed out changeset dd40d53329ea (Bug 1733580) for causing mochitest failures on browser_mouseButtonVariation.js.
Backout link
Push with failures
Failure Log

Comment 7

[BugBot [:suhaib / :marco/ :calixte]]

There's a r+ patch which didn't land and no activity in this bug for 2 weeks.
:pestanoa, could you have a look please?
For more information, please visit auto_nag documentation.

Comment 8

[Noah Pesta]

I am having a hard time troubleshooting the test failure found here. :mconley do you know why this might be the case?

Comment 9

[Mike Conley (:mconley) (:⚙️)]

Hi Noah,

Thanks for the patch! So a few things jump out at me:

1. Your patch modifies the "no controls" video UAWidget binding, but also needs to modify the "controls" version of it here: https://searchfox.org/mozilla-central/rev/553bf8428885dbd1eab9b63f71ef647f799372c2/toolkit/content/widgets/videocontrols.js#2792-2804
2. Applying your patch locally, it looks like the positioning of the toggle is affected by changing from a <button> to a <div>. We should ensure that the positioning is not affected by your patch.
3. It seems that switching from a <button> to a <div> changes some of the event characteristics within the UAWidget itself. Before your patch, mousedown'ing on the toggle, and the mouseup'ing on a different region of the page would result in the click event being suppressed here. How this works is by looking at the original target of the pointerdown event and comparing it with the target of the mouseup event - if they're different, then we know no click event is going to fire and we can skip attempting to suppress the click event.

With your patch applied, it looks like isMouseUpOnOtherElement is returning true for mouseup in the test, when before it didn't. You can find more history behind that test / logic where it originally landed here: https://bugzilla.mozilla.org/show_bug.cgi?id=1545296

What I'd actually recommend is option (1) from Gijs' comment 1 - keep the button, but try to eliminate the padding of the button.

Comment 10

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/6ff1d34c32c6
Fixed glitch on the PiP toggle icon in HCM. r=mtigley

Comment 11

[Marian-Vasile Laza]

https://hg.mozilla.org/mozilla-central/rev/6ff1d34c32c6

Comment 12

[Alin Ilea, Desktop QA]

Verified - Fixed in latest Nightly 96.0a1 (2021-12-02) using Windows 10 and Windows 7 with high contrast enabled.