Closed Bug 1449658 Opened 6 years ago Closed 6 years ago

about:preferences search still flickers and jumps the scrollbar when backspacing

Categories

(Firefox :: Settings UI, enhancement, P1)

Product:
Firefox
Component:
Settings UI
Type:
enhancement

Tracking

()

Status:
VERIFIED FIXED
Milestone:
Firefox 61
Tracking Flags:
Tracking Status
firefox61 --- fixed
firefox62 --- verified
firefox63 --- verified

People

(Reporter: jaws, Assigned: jaws)

References

Details

Attachments

(2 files)

Bug 1449658 - Elements that are already showing don't need their bindings force-applied since they're already applied. This removes the flicker when backspacing in the preference searchbox.
59 bytes, text/x-review-board-request
Gijs
: review+
Details
Bug 1449658 - Don't set hidden=true on items that are already visually-hidden when they don't match the search query since it is causing unnecessary layout changes.
59 bytes, text/x-review-board-request
Gijs
: review+
Details 
Followup to bug 1439930, we're hiding elements that are already visible (and matching the search) when we don't need to. This is causing the flicker and the scrollbar to move wildly.
Comment on attachment 8963265 [details]
Bug 1449658 - Elements that are already showing don't need their bindings force-applied since they're already applied. This removes the flicker when backspacing in the preference searchbox.

https://reviewboard.mozilla.org/r/232154/#review237680

r=me because this fixes flickering for me, but I still see the scrollbar (tested on macOS). Can we fix that, too?
Attachment #8963265 - Flags: review?(gijskruitbosch+bugs) → review+
Comment on attachment 8963804 [details]
Bug 1449658 - Don't set hidden=true on items that are already visually-hidden when they don't match the search query since it is causing unnecessary layout changes.

https://reviewboard.mozilla.org/r/232662/#review238126


Code analysis found 1 defect in this patch:
 - 1 defect found by mozlint

You can run this analysis locally with:
 - `./mach lint path/to/file` (JS/Python)


If you see a problem in this automated review, please report it here: http://bit.ly/2y9N9Vx


::: browser/components/preferences/in-content/findInPage.js:541
(Diff revision 1)
>  
>    /**
> -   * Remove all search tooltips that were created.
> +   * Remove all search tooltips.
>     */
>    removeAllSearchTooltips() {
> -    let searchTooltips = Array.from(document.querySelectorAll(".search-tooltip"));
> +    let listSearchTooltips = Array.from(document.querySelectorAll(".search-tooltip"));

Error: 'listsearchtooltips' is assigned a value but never used. [eslint: no-unused-vars]
Comment on attachment 8963804 [details]
Bug 1449658 - Don't set hidden=true on items that are already visually-hidden when they don't match the search query since it is causing unnecessary layout changes.

https://reviewboard.mozilla.org/r/232662/#review238184

::: browser/components/preferences/in-content/findInPage.js:283
(Diff revision 2)
>              groupHeader.hidden = false;
>            }
>  
>            resultsFound = true;
> -        } else if (!child.hidden) {
> +        } else if (!child.classList.contains("visually-hidden")) {
>            child.hidden = true;

I have to admit that I'm confused that this still sets hidden, but the other code now doesn't remove it. Doesn't this mean that if things don't match the search query, they still end up with `.hidden = true`, and then we don't remove it when they do start matching the search query? Not sure if I'm missing something... (leaving review request for now, but today + Monday are public holidays so not sure I can come back to this properly before Tuesday).
Blocks: 1446657
No longer blocks: 1439930
Comment on attachment 8963804 [details]
Bug 1449658 - Don't set hidden=true on items that are already visually-hidden when they don't match the search query since it is causing unnecessary layout changes.

https://reviewboard.mozilla.org/r/232662/#review238184

> I have to admit that I'm confused that this still sets hidden, but the other code now doesn't remove it. Doesn't this mean that if things don't match the search query, they still end up with `.hidden = true`, and then we don't remove it when they do start matching the search query? Not sure if I'm missing something... (leaving review request for now, but today + Monday are public holidays so not sure I can come back to this properly before Tuesday).

All `hidden` elements have `hidden` set to false on line 249. So by the time we get to line 282, we only need to hide elements that were visible previously. I'll upload a new patch now that makes this a bit clearer by using re-setting the visually-hidden class in this case.
Priority: -- → P1
Comment on attachment 8963804 [details]
Bug 1449658 - Don't set hidden=true on items that are already visually-hidden when they don't match the search query since it is causing unnecessary layout changes.

https://reviewboard.mozilla.org/r/232662/#review238696

Nice, thanks!

::: commit-message-0ada1:1
(Diff revision 4)
> +Bug 1449658 - Don't set hidden=true on items that are already visually-hidden when they don't match thesearch query since it is causing unnecessary layout changes. r?gijs

Nit: please split up `thesearch`
Attachment #8963804 - Flags: review?(gijskruitbosch+bugs) → review+
Blocks: 1450239
Pushed by jwein@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/78f7dd029128
Elements that are already showing don't need their bindings force-applied since they're already applied. This removes the flicker when backspacing in the preference searchbox. r=Gijs
https://hg.mozilla.org/integration/autoland/rev/50a505b2e51f
Don't set hidden=true on items that are already visually-hidden when they don't match the search query since it is causing unnecessary layout changes. r=Gijs
https://hg.mozilla.org/mozilla-central/rev/78f7dd029128
https://hg.mozilla.org/mozilla-central/rev/50a505b2e51f
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
status-firefox61: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 61
I have reproduced this bug with Nightly 61.0a1 (2018-03-28) on Windows 10 , 64 Bit ! 

This bug's fix is Verified with latest Nightly !

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

[bugday-20180404]
I verified the fix again using latest Nightly 63.0a1 and beta 62.0b7 on Windows 10 x64, Ubuntu 16.04 x64 and Mac OS 10.11. The bug is not reproducing anymore.
Status: RESOLVED → VERIFIED
status-firefox62: --- → verified
status-firefox63: --- → verified
Flags: in-qa-testsuite+
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: