Closed Bug 1393415 Opened 6 years ago Closed 6 years ago

Help icon isn't displayed with high contrast

Categories

(Firefox :: Settings UI, defect, P3)

Product:
Firefox
Component:
Settings UI
Version:
57 Branch
Type:
defect

Tracking

()

Status:
VERIFIED FIXED
Milestone:
Firefox 58
Tracking Flags:
Tracking Status
firefox55 --- unaffected
firefox56 --- unaffected
firefox57 --- verified
firefox58 --- verified

People

(Reporter: hani.yacoub, Assigned: rickychien)

References

(Blocks 1 open bug)

Details

(Whiteboard: [photon-preference])

Attachments

(3 files)

help icon.png
51.05 KB, image/png
Details
Bug 1393415 - Highlight help icon in high contrast theme
59 bytes, text/x-review-board-request
jaws
: review+
ritu
: approval-mozilla-beta+
Details
no-left-border.png
81.51 KB, image/png
Details
Attached image help icon.png — 
[Affected versions]: 
Nightly 57.0a1

[Affected platforms]:
Platforms: Windows 10 x 64 and Ubuntu 16.04 x64.

[Steps to reproduce]:
1. Launch Firefox.
2. Go to "about:preferences".
3. Observe the help icon.

[Expected result]:
Help icon should be displayed with high contrast.

[Actual result]:
Help icon isn't displayed with high contrast.
Blocks: 1357306
status-firefox55: --- → unaffected
status-firefox56: --- → unaffected
Whiteboard: [photon-preference][triage]
Priority: -- → P3
Whiteboard: [photon-preference][triage] → [photon-preference]
Flags: qe-verify+
QA Contact: hani.yacoub
Help icon is using background-image instead of list-style-image, causing the svg disappears in high contrast mode. This issue can be fixed by using list-style-image instead.
Assignee: nobody → rchien
Status: NEW → ASSIGNED
Not sure why `list-style-image` doesn't take effect on <html:a>, but using XUL element like image works well.

I'm going to replace entire <html:a> with XUL image in this case. Using XUL image will require rewriting original layout of <html:a>. Bug 1389550 will be addressed within this patch as well since `position: fixed` doesn't support well in XUL element, image will remain in category container in order to position properly.
Comment on attachment 8913161 [details]
Bug 1393415 - Highlight help icon in high contrast theme

https://reviewboard.mozilla.org/r/184582/#review190292

::: browser/components/preferences/in-content/preferences.xul
(Diff revision 5)
> +
>      <keyset>
>        <key key="&focusSearch1.key;" modifiers="accel" id="focusSearch1" oncommand="gSearchResultsPane.searchInput.focus();"/>
>      </keyset>
>  
> -    <html:a class="help-button" target="_blank" aria-label="&helpButton2.label;">&helpButton2.label;</html:a>

Instead of making the large change to XUL, I was able to get this working but inserting <html:img/> inside of the <html:a> tag. CSS `fill` works to change the color. I think this will be a much simpler fix.
Attachment #8913161 - Flags: review?(jaws) → review-
Comment on attachment 8913161 [details]
Bug 1393415 - Highlight help icon in high contrast theme

https://reviewboard.mozilla.org/r/184582/#review190292

> Instead of making the large change to XUL, I was able to get this working but inserting <html:img/> inside of the <html:a> tag. CSS `fill` works to change the color. I think this will be a much simpler fix.

Getting rid of `position: fixed` and appending help button in category sidebar element can bring more benefits such as

* Remove background-image and its background-position relavent code for RTL support.
* Using `list-style-image` in <image> to make svg highlightable in high contrast mode.
* we are able to remove init_dynamic_padding() to reduce sync reflow.

See also comment 2 and https://bugzilla.mozilla.org/show_bug.cgi?id=1389550#c3.
(In reply to Ricky Chien [:rickychien] from comment #10)
> Comment on attachment 8913161 [details]
> Bug 1393415 - Highlight help icon in high contrast theme
> 
> https://reviewboard.mozilla.org/r/184582/#review190292
> 
> > Instead of making the large change to XUL, I was able to get this working but inserting <html:img/> inside of the <html:a> tag. CSS `fill` works to change the color. I think this will be a much simpler fix.
> 
> Getting rid of `position: fixed` and appending help button in category
> sidebar element can bring more benefits such as
> 
> * Remove background-image and its background-position relavent code for RTL
> support.
> * Using `list-style-image` in <image> to make svg highlightable in high
> contrast mode.
> * we are able to remove init_dynamic_padding() to reduce sync reflow.
> 
> See also comment 2 and
> https://bugzilla.mozilla.org/show_bug.cgi?id=1389550#c3.

I don't think using a HTML link prevents you from removing position: fixed; and using the XUL spacer.
I would prefer a simpler patch assuming you want to uplift this to 57? Changing how we lay out the categories will be too risky to uplift to 57.
Good point. I'll use simpler fix instead in order to avoid uplift issue. And move this XUL layout patch to bug 1389550 for fx58 fix.
Comment on attachment 8913161 [details]
Bug 1393415 - Highlight help icon in high contrast theme

https://reviewboard.mozilla.org/r/184582/#review190506

::: browser/themes/shared/incontentprefs/preferences.inc.css:614
(Diff revision 7)
>    display: flex;
>    align-items: center;
>    position: fixed;
>    left: 0;
>    bottom: 36px;
> -  background-image: url("chrome://global/skin/icons/help.svg");
> +  background-image: none;

Isn't `background-image: none` default ?
Comment on attachment 8913161 [details]
Bug 1393415 - Highlight help icon in high contrast theme

https://reviewboard.mozilla.org/r/184582/#review190506

> Isn't `background-image: none` default ?

No, it's necessary. In order to override the help-glyph.svg of html|*.help-button from http://searchfox.org/mozilla-central/source/toolkit/themes/shared/in-content/common.inc.css#337
Comment on attachment 8913161 [details]
Bug 1393415 - Highlight help icon in high contrast theme

https://reviewboard.mozilla.org/r/184582/#review190510

::: browser/themes/shared/incontentprefs/preferences.inc.css:644
(Diff revision 7)
> +.help-icon {
> +  list-style-image: url("chrome://global/skin/icons/help.svg");
> +  margin-inline-start: -20px;
> +}
> +
> +@media (max-width: 830px) {
> +  .help-icon {
> +    margin-inline-start: 10px;
> +  }
> +}

Using .help-icon XUL image in order to add list-image-style attribute for support high constrast mode. Appending XUL element inside html:a caused some layout issue, so I have to add `margin-inline-start: -20px;` and `margin-inline-start: 10px;` trick for positioning help icon properly.
I have no idea why `list-style-image` doesn't take effect on <html:img>, but at least XUL image element does. As a result, using XUL <image> rather than <html:img> is the key workaround here.
(In reply to Ricky Chien [:rickychien] from comment #19)
> I have no idea why `list-style-image` doesn't take effect on <html:img>, but
> at least XUL image element does. As a result, using XUL <image> rather than
> <html:img> is the key workaround here.

Doesn't <html:img src="..." work ? list-style-image isn't supposed to work on <html:img> anyway, it's supported on XUL as an ugly hack, but it's not really standard for HTML
Yep, <html:img src=""> works! I totally forgot using src for loading resource. Thank you ntim! I've updated my patch.
Comment on attachment 8913161 [details]
Bug 1393415 - Highlight help icon in high contrast theme

https://reviewboard.mozilla.org/r/184582/#review190612

::: browser/components/preferences/in-content/preferences.xul:179
(Diff revision 8)
>      <keyset>
>        <key key="&focusSearch1.key;" modifiers="accel" id="focusSearch1" oncommand="gSearchResultsPane.searchInput.focus();"/>
>      </keyset>
>  
> -    <html:a class="help-button" target="_blank" aria-label="&helpButton2.label;">&helpButton2.label;</html:a>
> +    <html:a class="help-button" target="_blank" aria-label="&helpButton2.label;">
> +      <html:img src="chrome://global/skin/icons/help.svg"/>

Set the `width` and `height` attributes on the html:img to help with layout.

::: browser/themes/shared/incontentprefs/preferences.inc.css:642
(Diff revision 8)
> +.help-icon {
> +  list-style-image: url("chrome://global/skin/icons/help.svg");
> +}

This is dead code.

::: toolkit/themes/shared/in-content/common.inc.css
(Diff revision 8)
>    background-color: var(--in-content-category-background-selected-active);
>  }
>  
>  *|*#categories[keyboard-navigation="true"]:-moz-focusring > *|*.category[current] {
>    border: var(--in-content-category-border-focus);
> -  border-inline-start: none;

Why is this line removed?

::: toolkit/themes/shared/in-content/common.inc.css:739
(Diff revision 8)
> +  .help-label {
> +    display: none;
> +  }

Why does this need to be hidden for all consumers of common.inc.css? Doesn't .help-label only exist in preference code? And if it did exist outside of it, why would we want to hide it unconditionally?
Attachment #8913161 - Flags: review?(jaws) → review-
Comment on attachment 8913161 [details]
Bug 1393415 - Highlight help icon in high contrast theme

https://reviewboard.mozilla.org/r/184582/#review190612

> Why is this line removed?

While I was testing keyboard focus effect, I found that the outline of category is missing left-side-border. It's a simple fix for this regression. If you don't like that, I'm okay to file a new bug for that.
Comment on attachment 8913161 [details]
Bug 1393415 - Highlight help icon in high contrast theme

https://reviewboard.mozilla.org/r/184582/#review190912

::: toolkit/themes/shared/in-content/common.inc.css
(Diff revision 9)
>    background-color: var(--in-content-category-background-selected-active);
>  }
>  
>  *|*#categories[keyboard-navigation="true"]:-moz-focusring > *|*.category[current] {
>    border: var(--in-content-category-border-focus);
> -  border-inline-start: none;

Yeah, please put this line back. This line is necessary to match the UX specs for the correct dimensions.

::: toolkit/themes/shared/in-content/common.inc.css
(Diff revision 9)
>    .category {
>      padding-inline-start: 13px; /* make category icons align center */
>      margin-inline-end: 35px;
>    }
>  
> -  .help-button {

There's a good amount of CSS in common.inc.css that references `.help-button`. Are you sure that this would be safe to remove?
Attachment #8913161 - Flags: review?(jaws) → review-
Comment on attachment 8913161 [details]
Bug 1393415 - Highlight help icon in high contrast theme

https://reviewboard.mozilla.org/r/184582/#review190912

> There's a good amount of CSS in common.inc.css that references `.help-button`. Are you sure that this would be safe to remove?

Yes, I'm sure this can be removed safely. This .help-button was introduced during Photon visual refresh, but it's only used in preferences page. From now, it's useless after using <image> + <html:span> approach to deal with help button.

See also the blame history of this file http://searchfox.org/mozilla-central/source/toolkit/themes/shared/in-content/common.inc.css#732-738. This code is charge of question icon alignment, but we're using better <image> + <html:span> solution now.
Comment on attachment 8913161 [details]
Bug 1393415 - Highlight help icon in high contrast theme

https://reviewboard.mozilla.org/r/184582/#review190922

::: toolkit/themes/shared/in-content/common.inc.css
(Diff revision 9)
>    background-color: var(--in-content-category-background-selected-active);
>  }
>  
>  *|*#categories[keyboard-navigation="true"]:-moz-focusring > *|*.category[current] {
>    border: var(--in-content-category-border-focus);
> -  border-inline-start: none;

I'm not sure what you mean here. Photon visual spec has tweaked and introduced hover effect on category richlistitem. 

This line is supposed to be removed from that moment but we forgot. Put this line back will make focus ring look weird. See attachment.
Comment on attachment 8913161 [details]
Bug 1393415 - Highlight help icon in high contrast theme

https://reviewboard.mozilla.org/r/184582/#review191458
Attachment #8913161 - Flags: review?(jaws) → review+
Pushed by rchien@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/66452b85b5df
Highlight help icon in high contrast theme r=jaws
Comment on attachment 8913161 [details]
Bug 1393415 - Highlight help icon in high contrast theme

Approval Request Comment
[Feature/Bug causing the regression]: none
[User impact if declined]: minor, UI polish for windows high contrast mode
[Is this code covered by automated tests?]: no
[Has the fix been verified in Nightly?]: not yet
[Needs manual test from QE? If yes, steps to reproduce]: see description
[List of other uplifts needed for the feature/fix]: no
[Is the change risky?]: minor
[Why is the change risky/not risky?]: UI polish for windows high contrast mode
[String changes made/needed]: none
Attachment #8913161 - Flags: approval-mozilla-beta?
https://hg.mozilla.org/mozilla-central/rev/66452b85b5df
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
status-firefox58: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 58
Comment on attachment 8913161 [details]
Bug 1393415 - Highlight help icon in high contrast theme

Photon polish, Beta57+
Attachment #8913161 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Build ID: 20171005100211
User Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:58.0) Gecko/20100101 Firefox/58.0

Verified as fixed on Firefox Nightly 58.0a1 on Windows 10 x 64, Windows 7 x32 and Ubuntu 16.04 x64.
status-firefox58: fixed → verified
Build ID: 20171005195903
User Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:57.0) Gecko/20100101 Firefox/57.0

Verified as fixed on Firefox Beta 57.0b6 on Windows 10 x 64, Windows 7 x32 and Ubuntu 16.04 x64.
status-firefox57: fixed → verified
Status: RESOLVED → VERIFIED
Flags: qe-verify+
Flags: qe-verify+
You need to log in before you can comment on or make changes to this bug.