Closed Bug 1587127 Opened 5 years ago Closed 5 years ago

Ensure HCM backplate doesn't backplate items with visibility:hidden

Categories

(Core :: Layout, defect, P1)

defect

Tracking

()

RESOLVED FIXED
mozilla72
Tracking Status
firefox-esr68 --- unaffected
firefox70 --- unaffected
firefox71 + fixed
firefox72 --- fixed

People

(Reporter: morgan, Assigned: morgan)

References

(Regression)

Details

(Keywords: access, regression)

Attachments

(6 files)

Attached image backplate_windows_actual.png (deleted) —

STR:

  1. Enable windows high contrast mode, or navigate to settings > colors > enable high contrast and set the dropdown menu to Always
  2. Ensure permit_backplate in about:config is set to True.
  3. Visit https://www.google.com/search?client=firefox-b-1-d&q=test

Expected:

  • Text in the search bar is backplated

Actual:

  • Text in the search bar is backplated
  • Portions of the results are obscured by backplate boxes
Attached image backplate_mac_actual.png (deleted) —
Keywords: access
Priority: -- → P3

Bug 1591431 was marked as a duplicate so this is a tracked regression affecting search for 71.

Changing the priority to p1 as the bug is tracked by a release manager for the current beta.
See What Do You Triage for more information

Priority: P3 → P1
Attachment #9099934 - Attachment description: Bug 1587127: Add casing, tests for visibility:hidden items when backplating. → Bug 1587127: Ensure no backplate is drawn for lines that have only visibility:hidden descendants.
Pushed by mreschenberg@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/c20f9ebdd5c0 Ensure no backplate is drawn for lines that have only visibility:hidden descendants. r=dholbert
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla72

Please nominate this patch for Beta approval when you get a chance.

Flags: needinfo?(mreschenberg)

Verified fixed on MacOS 10.14.6, Nightly 20191031194708 :)
Working on uplift today

Flags: needinfo?(mreschenberg)

Comment on attachment 9099934 [details]
Bug 1587127: Ensure no backplate is drawn for lines that have only visibility:hidden descendants.

Beta/Release Uplift Approval Request

  • User impact if declined: If declined, high contrast mode (HCM) users will be unable to correctly view pages with hidden elements (large impact: google searches).
  • Is this code covered by automated tests?: Yes
  • Has the fix been verified in Nightly?: Yes
  • Needs manual test from QE?: No
  • If yes, steps to reproduce:
  • List of other uplifts needed: None
  • Risk to taking this patch: Medium
  • Why is the change risky/not risky? (and alternatives if risky): The logic for checking element visibility is a little complex (nsBlockFrame.cpp changes), but this change effects only elements that are being handled incorrectly in HCM currently. If regressions are to occur, it is likely they'll occur with other edge cases of visibility (though we've already covered display:none, opacity:0, and in this patch visibility:hidden)
  • String changes made/needed:

Please ?ni me with questions :)

Attachment #9099934 - Flags: approval-mozilla-beta?
Flags: qe-verify+

Comment on attachment 9099934 [details]
Bug 1587127: Ensure no backplate is drawn for lines that have only visibility:hidden descendants.

P1 tracked bug with a high a11y impact, patch with tests, uplift approved for 71 beta 7, let's get it verified by QA with the steps in comment #0. Thanks.

Attachment #9099934 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
QA Whiteboard: [qa-triaged]

The fix does not seem to have fixed the issue for me. Attempted verification in Windows 10 and Mac OS 10.13.6, Nightly v72.0a1 (2019-11-04).

These are my steps:

  1. Go to about:preferences#general, Fonts and Colors section, click on the "Colors..." button.
  2. Click on "Override the colors specified by the page with your selections above" drop-down and select "Always" and save settings.
  3. Go to: www.google.com
    Actual: Content is visible and readable.
    Expected: Contend is covered by some blank frame.
  4. Go to a search page: https://www.google.com/search?client=firefox-b-1-d&q=test
    Actual: Content is visible and readable.
    Expected: Contend is covered by some blank frame.

From my tests, I notice that Windows (or Mac OS) High Contrast does not even need to be activated for the issue to reproduce. (Using it does not change the issue's reproduction AND the "browser.display.permit_backplate" is true by default)

Did I understand the issue correctly? Did I perform the steps correctly?

Flags: needinfo?(mreschenberg)

Hello!

I'm a bit confused by your expected/actual statements. Without the patch applied (but with HCM and backplate enabled), google search results have extra frames obscuring the results. Note that this issue is not apparent on the google.com homepage, but on results. This is the "actual" result.

The "expected" result is content should render as normal; no frames should obscure results. Text in the google search bar should be backplated if the user has set a backplate colour that differs from google's background colour.

Does this make sense?

Flags: needinfo?(mreschenberg) → needinfo?(daniel.bodea)

The actual and expected results in comment 13 were accidentally switched (typo), sorry.

Anyhow, I have retested the issue with THESE steps:

  1. Activate Windows' feature, High Contrast mode.
  2. Ensure "browser.display.permit_backplate" pref in about:config is set to True. (By default: true in Nightly and false in Beta)
  3. Go to: https://www.google.com/search?client=firefox-b-1-d&q=test
    Expected: Content is visible and readable.
    Actual: Contend is covered by some blank frame.

Results:

  • Affected builds:
  1. Nightly v71.0a (2019-10-08) -> issue occurs
  2. Beta v71.0b6 -> issue occurs
  • APPARENTLY fixed builds:
  1. Nightly v72.0a1 (2019-11-04) -> issue still occurs
    (a new changeset was pushed to inbound in comment 15, maybe something was missing from the push?)
  2. Beta v71.0b7 -> issue still occurs here too
    (please note that if "browser.display.permit_backplate" pref is left false (default), the issue appears fixed.)

I will wait for the inbound push to get updated in the latest Nightly and retest. Thank you.

(In reply to Bodea Daniel [:danibodea] from comment #16)

(a new changeset was pushed to inbound in comment 15, maybe something was missing from the push?)
[...]
I will wait for the inbound push to get updated in the latest Nightly and retest. Thank you.

I don't actually know what comment 15 was about -- that seems to have been posted by mistake. It's the exact same commit-hash from comment 6, and it shows datestamp Oct 30th if you click through, with Oct 31 as the date for its push [to m-i] if you click through its "push id" link. So there's nothing new there. I'm going to mark it as "offtopic" to auto-hide it since it's not relevant and was likely posted by mistake (or at least on a many-day delay) and it causes confusion about when things happened here.

I'm also confused -- you said:

APPARENTLY fixed builds:
Nightly v72.0a1 (2019-11-04) -> issue still occurs

So to clarify, you are seeing the issue happen in that build? (and by "apparently fixed" you mean the build-is-after-the-fix-date, but the issue's not fixed?)

Could you post a screenshot of the issue that you're seeing there, just to sanity check that it's the same problem?

I tested locally on Win10 and I'm not seeing the issue in recent nightlies (e.g. the latest nightly as well as the one from Nov 1st).

(In reply to Daniel Holbert [:dholbert] from comment #17)

I tested locally on Win10 and I'm not seeing the issue in recent nightlies (e.g. the latest nightly as well as the one from Nov 1st).

Also: I verified that I do see the issue in slightly older builds from e.g. 10/30 [not quite as much covered up as in comment 0's screenshot, but definitely some pieces of words in result-summaries are missing]

I ran mozregression-gui with the "find-fix" setting and it narrowed to this bug's push. So, my results are consistent with the issue going away due to this fix landing, in nightlies, as-expected.

For the record, here's how this issue looks for me, in nightly builds that predate the fix landing. (Notice the random stomped-on chunks of descriptive text below the results. Similar to comment 0, though the stomped areas are smaller for me.

In nightly builds after this was fixed, the text is all visible for me; nothing is stomped.

This is how the Google results page is displayed on the latest Nightly while Windows' High Contrast mode activated.

Flags: needinfo?(daniel.bodea)
Attached image betav71.0b7reproduces.png (deleted) —

This is how the search results page is displayed in Beta v71.0b7 when the Windows' High Contrast mode is activated AND "browser.display.permit_backplate" pref is flipped to "True".

The same happens to the affected builds (before the fix) using the steps in comment 16. How should I proceed?

Flags: needinfo?(dholbert)

Thanks for the screenshots. It sounds like something is still very wrong here, though I can't reproduce locally, so it's hard for me to know what's going on.

As for how to proceed: I'd rather not reopen this bug & do more work here, since we already had some known-good/helpful patches land here.

Would you mind cloning this bug (via New/Clone button at top right), and titling it something like "HCM backplate is covering up large regions of Google Search Results page", and including the STR that you're using and a screenshot?

(Fortunately, we don't have to worry too much about this affecting release users, because the HCM Backplate feature is set to automatically disable after the early-beta period. Depending on how easy the issue is to trigger vs. how quickly we can figure it out, we may want to restrict the feature even further, since those screenshots look pretty usability-breaking.)

Flags: needinfo?(dholbert) → needinfo?(daniel.bodea)

Considering that (apparently) another issue was found (bug 1595122) regarding this bug's exact steps to reproduce, it will be addressed there further and this one will be verified after the other is fixed due to being blocked. This being said, I will remove the qe-verify+ tag.

Flags: qe-verify+
Flags: needinfo?(daniel.bodea)
Has Regression Range: --- → yes
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: