1390499 - No appropriate header when preference search shows permission items

Status: VERIFIED FIXED

(Firefox :: Settings UI, defect, P1)

Description

[:Gijs (he/him)]

STR:

1. open about:preferences
2. search for "settings"
3. scroll down

ER:

Location/Camera/Microphone/Notifications show a "Permissions" header above themselves

AR:

no header shows up


A similar problem shows up for the "block popup windows" and "warn you when websites try to install add-ons". Especially in the last case, the sentence doesn't make a lot of sense without the prefix to the checkbox, which is probably something like "Firefox should", but isn't shown.

Comment 2

[Jared Wein [:jaws] (please needinfo? me)]

Looking at the Firefox Account page, we probably just need to duplicate the title like is done there.

If we wanted to go the extra step, we could hide subtitles when they are equal to their section header, but only when the section header is visible.

Comment 3

[Johann Hofmann [:johannh]]

I find it a bit ugly to (In reply to Jared Wein [:jaws] (please needinfo? me) from comment #2)
> Looking at the Firefox Account page, we probably just need to duplicate the
> title like is done there.

Yes, I think that's the most pragmatic solution for now!

> If we wanted to go the extra step, we could hide subtitles when they are
> equal to their section header, but only when the section header is visible.

I'd love if we could do this sooner rather than later as IMO the double header looks a bit strange for Firefox Accounts. Maybe we could then also fix up the other sections that don't show a subtitle.

Comment 4

[Evan Tseng [:evanxd]]

It is also a design issue[1]. We hide the level one headers in Bug 1382134 because search result page already has the "Search Results" level one header and showing other level one headers might confuse users (Users might feel there are other settings not related with the search keyword).

[1]: https://bugzilla.mozilla.org/show_bug.cgi?id=1382134#c1

Comment 5

[Ricky Chien [:rickychien] (inactive)]

Attachment: Bug 1390499 - Show all second level headers in Preferences search result

Review commit: https://reviewboard.mozilla.org/r/169604/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/169604/

Comment 6

[Ricky Chien [:rickychien] (inactive)]

Comment on attachment 8898238 [details]
Bug 1390499 - Show all second level headers in Preferences search result

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/169604/diff/1-2/

Comment 7

[Ricky Chien [:rickychien] (inactive)]

After discussion with Helen, we came out with a programatic solution which introduces hidden second level headers for `Nightly Update`, `Performance`, `Browsing`, `Network Proxy`, `Permissions`, `Nightly Data Collection and Use` sections. And then unhide those 2nd level headers in search result page.

This is a nice and simple way to fix and avoid user confused. Jared, can you help review this patch? thanks

Comment 8

[Jared Wein [:jaws] (please needinfo? me)]

Comment on attachment 8898238 [details]
Bug 1390499 - Show all second level headers in Preferences search result

https://reviewboard.mozilla.org/r/169604/#review174942

r=me with the following lines removed

::: browser/components/preferences/in-content-new/findInPage.js:244
(Diff revision 2)
>        let rootPreferencesChildren = document
>          .querySelectorAll("#mainPrefPane > *:not([data-hidden-from-search])");
>  
> +      // Show all second level headers in search result
> +      for (let element of document.querySelectorAll("caption.search-header")) {
> +        if (element.classList.contains("search-header")) {

This line is redudant since the querySelectorAll above will only return elements that already have the "search-header" class.

::: browser/components/preferences/in-content-new/findInPage.js:331
(Diff revision 2)
>        // Going back to General when cleared
>        gotoPref("paneGeneral");
> +
> +      // Hide some special second level headers in normal view
> +      for (let element of document.querySelectorAll("caption.search-header")) {
> +        if (element.classList.contains("search-header")) {

This line is also redundant.

Comment 9

[Ricky Chien [:rickychien] (inactive)]

Comment on attachment 8898238 [details]
Bug 1390499 - Show all second level headers in Preferences search result

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/169604/diff/2-3/

Comment 10

[Ricky Chien [:rickychien] (inactive)]

Comment on attachment 8898238 [details]
Bug 1390499 - Show all second level headers in Preferences search result

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/169604/diff/3-4/

Comment 11

[Ricky Chien [:rickychien] (inactive)]

Comment on attachment 8898238 [details]
Bug 1390499 - Show all second level headers in Preferences search result

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/169604/diff/4-5/

Comment 12

[Ricky Chien [:rickychien] (inactive)]

I've no idea why try result threw errors like:

missing chrome://browser/skin/notification-icons/geo-linux.svg referenced from chrome://browser/skin/preferences/in-content-new/privacy.css

In order to prove that failures are irrelevant to my patch, I've pushed an another clear try without changing anything. As you can see, the same errors are still happening.

Clear try result (master branch):

https://treeherder.mozilla.org/#/jobs?repo=try&revision=84714cd7f099a7dfc23006f0e0a6ca7328fc5289&selectedJob=124031540

Error try result (my patch):

https://treeherder.mozilla.org/#/jobs?repo=try&revision=763741d288d302ef67523d306c758baccfc1733b&selectedJob=124040521


I'm going to trigger autoland since above observation tells the try failures might be irrelevant.

Comment 13

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/2f5692b521da
Show all second level headers in Preferences search result r=jaws

Comment 14

[Johann Hofmann [:johannh]]

(In reply to Ricky Chien [:rickychien] from comment #12)
> I've no idea why try result threw errors like:
> 
> missing chrome://browser/skin/notification-icons/geo-linux.svg referenced
> from chrome://browser/skin/preferences/in-content-new/privacy.css
> 
> In order to prove that failures are irrelevant to my patch, I've pushed an
> another clear try without changing anything. As you can see, the same errors
> are still happening.
> 
> Clear try result (master branch):
> 
> https://treeherder.mozilla.org/#/
> jobs?repo=try&revision=84714cd7f099a7dfc23006f0e0a6ca7328fc5289&selectedJob=1
> 24031540
> 
> Error try result (my patch):
> 
> https://treeherder.mozilla.org/#/
> jobs?repo=try&revision=763741d288d302ef67523d306c758baccfc1733b&selectedJob=1
> 24040521
> 
> 
> I'm going to trigger autoland since above observation tells the try failures
> might be irrelevant.

Yeah it looks like something went wrong in bug 1371230. I'm filing a bug for that.

Comment 15

[Johann Hofmann [:johannh]]

Filed bug 1391550.

Comment 16

[Ricky Chien [:rickychien] (inactive)]

(In reply to Johann Hofmann [:johannh] from comment #15)
> Filed bug 1391550.

Thanks:)

Comment 17

[Ricky Chien [:rickychien] (inactive)]

Comment on attachment 8898238 [details]
Bug 1390499 - Show all second level headers in Preferences search result

Approval Request Comment
[Feature/Bug causing the regression]: regression from bug 1365133
[User impact if declined]: keyboard shortcut behavior doesn't work as expected.
[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]: yes. see description
[List of other uplifts needed for the feature/fix]: n/a
[Is the change risky?]: no
[Why is the change risky/not risky?]: small UX regression from bug 1365133
[String changes made/needed]: no

Comment 18

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/2f5692b521da

Comment 19

[Gerry Chang [:gchang]]

Hi Brindusa, could you help find someone to verify if this issue was fixed as expected on the latest Nightly build? Thanks!

Comment 20

[Gerry Chang [:gchang]]

Comment on attachment 8898238 [details]
Bug 1390499 - Show all second level headers in Preferences search result

A UX polishment. Beta56+.

Comment 21

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/0584276b1f58

Comment 22

[Brindusa Tot, Desktop QA]

Camelia, please take a look and verify this on latest Nightly and Beta builds.

Comment 23

[Anika Alam Raha]

I have reproduced this bug with Nightly 57.0a1 (2017-08-15) on Windows 8, 64-bit.

The bug's fix is now verified on Latest nightly 57.0a1 

Build ID 	20170904220027
User Agent 	Mozilla/5.0 (Windows NT 6.3; Win64; x64; rv:57.0) Gecko/20100101 Firefox/57.0

[bugday-20170906]

Comment 24

[Hani Yacoub, Desktop QA]

Build ID: 20170918220054
User Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:57.0) Gecko/20100101 Firefox/57.0

Verified as fixed on Firefox Nightly 57.0a1 on Windows 10 x 64, Mac OS X 10.12 and Ubuntu 16.04 x64.