Should adapt the oboarding UI to the hight-contrast display mode

VERIFIED FIXED in Firefox 56

Status

()

Firefox
New Tab Page
P1
normal
VERIFIED FIXED
2 months ago
18 days ago

People

(Reporter: Fischer, Assigned: Fischer)

Tracking

55 Branch
Firefox 56
Points:
---
Dependency tree / graph
Bug Flags:
qe-verify +

Firefox Tracking Flags

(firefox56 fixed)

Details

(Whiteboard: [photon-onboarding])

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(10 attachments)

(Assignee)

Description

2 months ago
SOP:
- Set `browser.display.document_color_use=2` to enable the high-contrast mode
- Open about:newtab or about:home

Expected result:
The images of the Onboarding tours exist

Actual result:
The images of the Onboarding tours disappear

This is because in high-contrast mode the background images specified by author stylesheet would be ignored. See [1] for details.

[1] https://bugzilla.mozilla.org/show_bug.cgi?id=1022553#c2
(Assignee)

Updated

2 months ago
Assignee: nobody → fliu
Status: NEW → ASSIGNED
Priority: -- → P2
Whiteboard: [photon-onboarding]
Target Milestone: --- → Firefox 57

Updated

2 months ago
Flags: qe-verify+
QA Contact: jwilliams
(Assignee)

Updated

a month ago
Summary: Should not use background-image to display image → Should adapt the oboarding UI to the hight-contrast display mode
(Assignee)

Comment 1

a month ago
Created attachment 8884150 [details]
not_handle_high_contrast_mode_1.png
(Assignee)

Comment 2

a month ago
Created attachment 8884151 [details]
not_handle_high_contrast_mode_2.png
(Assignee)

Comment 3

a month ago
Created attachment 8884152 [details]
handle_high_contrast_mode_when_disabling_high_contrast_mode_1.png
(Assignee)

Comment 4

a month ago
Created attachment 8884154 [details]
handle_high_contrast_mode_when_disabling_high_contrast_mode_2.png
(Assignee)

Comment 5

a month ago
Created attachment 8884155 [details]
handle_high_contrast_mode_when_enabling_high_contrast_mode_1.png
(Assignee)

Comment 6

a month ago
Created attachment 8884156 [details]
handle_high_contrast_mode_when_enabling_high_contrast_mode_2.png
(Assignee)

Comment 7

a month ago
Created attachment 8884159 [details]
handle_high_contrast_mode.gif
(Assignee)

Comment 8

a month ago
Hi Verdi,
Per the discussion during the SF workweek, we should make the most important and the minimum UI ingredients visible under the high-contrast display mode. Please see the UI for the high-contrast display mode:
- attachment 8884155 [details]: handle_high_contrast_mode_when_enabling_high_contrast_mode_1.png
- attachment 8884156 [details]: handle_high_contrast_mode_when_enabling_high_contrast_mode_2.png
- attachment 8884159 [details]: handle_high_contrast_mode.gif

You should see
- buttons now would have outline
- the upper-left fox icon is visible
- the X close buttons on the notification bar and on the overlay are visible
- the navigation item of the current tour on the overlay's right side would have outline
- the hovered navigation item on the overlay's right side would have outline

Please confirm the UI. If ok, we will go for this implementation, thank you.
Flags: needinfo?(mverdi)
(Assignee)

Updated

a month ago
Attachment #8884159 - Flags: ui-review?(mverdi)
Are you sure this is not 56?
Target Milestone: Firefox 57 → Firefox 56
Ni for above question.
Flags: needinfo?(fliu)
Priority: P2 → P1
(Assignee)

Comment 11

a month ago
(In reply to Tim Guan-tin Chien [:timdream] (please needinfo) from comment #10)
> Ni for above question.
Thanks for updated this. I guess this was a mistake while creating bugs.
Flags: needinfo?(fliu)

Comment 12

a month ago
This is looking good.

I have one concern about the buttons "Learn More" (in the notifications) and "Show Private Browsing in Menu", etc. in the overlay. I think they can be higher contrast. Either black or maybe they should be blue like links and other web buttons are (https://www.mozilla.org/en-US/firefox/new/ for example). I'm not sure of the convention here. Can we check with Yura?
Flags: needinfo?(mverdi) → needinfo?(yzenevich)
(In reply to Verdi [:verdi] from comment #12)
> This is looking good.
> 
> I have one concern about the buttons "Learn More" (in the notifications) and
> "Show Private Browsing in Menu", etc. in the overlay. I think they can be
> higher contrast. Either black or maybe they should be blue like links and
> other web buttons are (https://www.mozilla.org/en-US/firefox/new/ for
> example). I'm not sure of the convention here. Can we check with Yura?

Yeah I would agree that black would make it great. There are a11y guidelines that are related to contrast and text, but not so much to things like borders and background. I guess since it's a high contrast mode, it we should ideally avoid lower contrast for those buttons (it looks like it would also be pretty consistent with the tab list buttons on the left). Here's a checker that might be useful for checking contrast (I guess for high contrast we would need AAA pass): http://webaim.org/resources/contrastchecker/
Flags: needinfo?(yzenevich)
(Assignee)

Comment 14

a month ago
Created attachment 8885683 [details]
button_grey_outline_under_hight_contrast_mode.png
(Assignee)

Comment 15

a month ago
Created attachment 8885684 [details]
div_as_button_black_outline_under_hight_contrast_mode.png
(Assignee)

Comment 16

a month ago
It seems the grey border for a <button> under the high-contrast mode is the default style. Even set black border on the buttons in the bottom of the about:home page, we still got the grey border under the high-contrast mode. See attachment 8885683 [details]: button_grey_outline_under_hight_contrast_mode.png.

A simple solution is to utilize a <div> as a <button>. If that is a <div>, then the border would be black, which is clearer and more contrast, under the high-contrast mode. See attachment 8885684 [details]: div_as_button_black_outline_under_hight_contrast_mode.png.
yes please do not use divs as buttons or links. this makes the whole thing inaccessible as you are removing semantics that screen reader and other assistive technologies use. 

Couple of suggestions: perhaps CSS selector could be more explicit (to override the default rule).
I would also check that this is not an outline that is showing.
In case all of that fails, I would try resetting appearance property to none (https://developer.mozilla.org/en/docs/Web/CSS/-moz-appearance) and styling it the way we would want it to be.
(Assignee)

Comment 18

a month ago
Per the discussion with UX: verdi on the meeting, we can go for the given default grey button border in the high-contrast mode. 

About the black button border, currently haven't found a css way for it. Not sure if a grey border is to distinguish button from other elements. Anyway we could investigate this in another bug. For now better to have the onboarding tours handle the high-contrast mode.
(Assignee)

Updated

a month ago
Blocks: 1377273
Comment hidden (mozreview-request)
(Assignee)

Comment 20

a month ago
(In reply to Fischer [:Fischer] from comment #19)
> Created attachment 8887356 [details]
> Bug 1377439 - Should adapt the oboarding UI to the hight-contrast display
> mode,
> 
> This patch makes the mininmum and the important elements and images visible
> in the high-constrast mode, including
> - making button have border (using the given default grey border in the
> high-constrast mode)
> - making the upper-left overlay button's fox image visible
> - making the images of the X close buttons on the notification bar and on
> the overlay visible
> - making that the navigation item of the current tour on the overlay's right
> side would have outline
> - making the hovered navigation item on the overlay's right side would have
> outline
> - making sure using <button> element for buttons for a better semantic
> - changing #onboarding-overlay-icon to #onboarding-overlay-button for a
> better semantic
> 
> Review commit: https://reviewboard.mozilla.org/r/158184/diff/#index_header
> See other reviews: https://reviewboard.mozilla.org/r/158184/
Hi Mossop,

Please see 
- attachment 8884155 [details]: handle_high_contrast_mode_when_enabling_high_contrast_mode_1.png
- attachment 8884156 [details]: handle_high_contrast_mode_when_enabling_high_contrast_mode_2.png
- attachment 8884159 [details]: handle_high_contrast_mode.gif
for the ui.

thank you.
Comment hidden (mozreview-request)

Comment 22

a month ago
mozreview-review
Comment on attachment 8887356 [details]
Bug 1377439 - Should adapt the oboarding UI to the hight-contrast display mode,

https://reviewboard.mozilla.org/r/158184/#review164372
Attachment #8887356 - Flags: review?(dtownsend) → review+

Comment 23

a month ago
mozreview-review
Comment on attachment 8887356 [details]
Bug 1377439 - Should adapt the oboarding UI to the hight-contrast display mode,

https://reviewboard.mozilla.org/r/158184/#review164520

::: browser/extensions/onboarding/content/onboarding.js:708
(Diff revision 2)
> -    img.id = "onboarding-overlay-icon";
> -    return img;
> +    button.id = "onboarding-overlay-button";
> +    let img = this._window.document.createElement("img");
> +    img.id = "onboarding-overlay-button-icon";
> +    img.src = "resource://onboarding/img/overlay-icon.svg";
> +    button.appendChild(img);
> +    return button;

please rebase since rex has landed the welcome message for fox icon
(Assignee)

Comment 24

a month ago
Need to rebase on and check-in after the bug 1380963 and 1357017.
Depends on: 1380963, 1357017
Comment hidden (mozreview-request)
(Assignee)

Comment 26

a month ago
TRY: https://treeherder.mozilla.org/#/jobs?repo=try&revision=dc97dd5f6ec5f61a29584478517884ecc8105612
Keywords: checkin-needed
(Assignee)

Comment 27

a month ago
Remove checkin-needed because the bug 1380963 is not yet checked in
Keywords: checkin-needed

Comment 28

29 days ago
We're sorry, Autoland could not rebase your commits for you automatically. Please manually rebase your commits and try again.

hg error in cmd: hg rebase -s 927658d278dc -d 4df8bfcb4e9f: rebasing 408171:927658d278dc "Bug 1377439 - Should adapt the oboarding UI to the hight-contrast display mode, r=mossop" (tip)
merging browser/extensions/onboarding/content/onboarding.css
merging browser/extensions/onboarding/content/onboarding.js
warning: conflicts while merging browser/extensions/onboarding/content/onboarding.js! (edit, then use 'hg resolve --mark')
unresolved conflicts (see hg resolve, then hg rebase --continue)
(Assignee)

Comment 29

29 days ago
Checkin-needed again because the bug 1380963 was checked in
Keywords: checkin-needed

Comment 30

29 days ago
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/e9b1c60ebd4f
Should adapt the oboarding UI to the hight-contrast display mode, r=mossop
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/e9b1c60ebd4f
Status: ASSIGNED → RESOLVED
Last Resolved: 29 days ago
status-firefox56: --- → fixed
Resolution: --- → FIXED

Updated

26 days ago
Blocks: 1381360
I have verified this bug on today's nightly.
Status: RESOLVED → VERIFIED

Updated

18 days ago
Blocks: 1385123
You need to log in before you can comment on or make changes to this bug.