Closed
Bug 1390499
Opened 7 years ago
Closed 7 years ago
No appropriate header when preference search shows permission items
Categories
(Firefox :: Settings UI, defect, P1)
Firefox
Settings UI
Tracking
()
VERIFIED
FIXED
Firefox 57
Tracking | Status | |
---|---|---|
firefox-esr52 | --- | unaffected |
firefox55 | --- | unaffected |
firefox56 | --- | fixed |
firefox57 | --- | verified |
People
(Reporter: Gijs, Assigned: rickychien)
References
(Blocks 1 open bug)
Details
(Whiteboard: [photon-preference])
Attachments
(1 file)
59 bytes,
text/x-review-board-request
|
jaws
:
review+
gchang
:
approval-mozilla-beta+
|
Details |
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•7 years ago
|
||
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.
Updated•7 years ago
|
Blocks: photon-preference
Whiteboard: [photon-preferences] [triage] → [photon-preferences][triage]
Comment 3•7 years ago
|
||
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•7 years ago
|
||
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
Assignee | ||
Updated•7 years ago
|
Assignee: nobody → rchien
Status: NEW → ASSIGNED
Flags: qe-verify+
Priority: -- → P1
QA Contact: hani.yacoub
Target Milestone: --- → Firefox 57
Assignee | ||
Updated•7 years ago
|
status-firefox55:
--- → unaffected
status-firefox56:
--- → affected
status-firefox-esr52:
--- → unaffected
Updated•7 years ago
|
Whiteboard: [photon-preferences][triage] → [photon-preferences]
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Comment 7•7 years ago
|
||
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•7 years ago
|
||
mozreview-review |
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.
Attachment #8898238 -
Flags: review?(jaws) → review+
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Comment 12•7 years ago
|
||
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•7 years ago
|
||
Pushed by rchien@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/2f5692b521da Show all second level headers in Preferences search result r=jaws
Comment 14•7 years ago
|
||
(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•7 years ago
|
||
Filed bug 1391550.
Assignee | ||
Comment 16•7 years ago
|
||
(In reply to Johann Hofmann [:johannh] from comment #15) > Filed bug 1391550. Thanks:)
Assignee | ||
Comment 17•7 years ago
|
||
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
Attachment #8898238 -
Flags: approval-mozilla-beta?
Comment 18•7 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/2f5692b521da
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Comment 19•7 years ago
|
||
Hi Brindusa, could you help find someone to verify if this issue was fixed as expected on the latest Nightly build? Thanks!
Flags: needinfo?(brindusa.tot)
Comment 20•7 years ago
|
||
Comment on attachment 8898238 [details] Bug 1390499 - Show all second level headers in Preferences search result A UX polishment. Beta56+.
Attachment #8898238 -
Flags: approval-mozilla-beta? → approval-mozilla-beta+
Comment 21•7 years ago
|
||
bugherder uplift |
https://hg.mozilla.org/releases/mozilla-beta/rev/0584276b1f58
Comment 22•7 years ago
|
||
Camelia, please take a look and verify this on latest Nightly and Beta builds.
Flags: needinfo?(brindusa.tot)
QA Contact: hani.yacoub → camelia.badau
Comment 23•7 years ago
|
||
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•7 years ago
|
||
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.
Status: RESOLVED → VERIFIED
Updated•6 years ago
|
Whiteboard: [photon-preferences] → [photon-preference]
You need to log in
before you can comment on or make changes to this bug.
Description
•