Closed Bug 1534329 Opened 9 months ago Closed 9 months ago

Preferences' findInPage.js seems to do unnecessary work processing and hiding elements that don't have visual representations

Categories

(Firefox :: Preferences, defect, P3)

defect

Tracking

()

RESOLVED FIXED
Firefox 68
Tracking Status
firefox68 --- fixed

People

(Reporter: Kwan, Assigned: nimbus2020b, Mentored, NeedInfo)

References

Details

(Keywords: good-first-bug)

Attachments

(1 file)

As discovered when diagnosing bug 1532931, when examining the DOM after using the search box in preferences, one notices that <script> and <stringbundle> elements have had the class visually-hidden added. This suggests that the findInPage.js script is wasting time processing these elements when performing searches, and this is indeed the case, since one can observe via the debugger <script>s being processed by the loop over child elements [0] and thus the whole of the searchWithinNode function [1].
The simplest fix may be to add some :not()s with said tagNames to the selector [2]. Seems like good-first-bug material.

[0] https://searchfox.org/mozilla-central/rev/2f1020dc4176d38dd5f3d0496f3c46849862ee0b/browser/components/preferences/in-content/findInPage.js#255-283
[1] https://searchfox.org/mozilla-central/rev/2f1020dc4176d38dd5f3d0496f3c46849862ee0b/browser/components/preferences/in-content/findInPage.js#331-431
[2] https://searchfox.org/mozilla-central/rev/2f1020dc4176d38dd5f3d0496f3c46849862ee0b/browser/components/preferences/in-content/findInPage.js#235

Thanks for filing.

(In reply to Ian Moody [:Kwan] (UTC+0) from comment #0)

The simplest fix may be to add some :not()s with script and stringbundle to the selector in findInPage.js.

Yep. Let's mark this as a good first bug.

Mentor: gijskruitbosch+bugs
Keywords: good-first-bug
Priority: -- → P3

Hey! I would like to work on this issue can you assign it to me?

(In reply to Nidhi Kumari from comment #2)

Hey! I would like to work on this issue can you assign it to me?

Sure.

Assignee: nobody → nimbus2020b

(In reply to :Gijs (he/him) from comment #3)

(In reply to Nidhi Kumari from comment #2)

Hey! I would like to work on this issue can you assign it to me?

Sure.

Are we not supposed to select any script or stringbundle? If so, we can just do body:not(script, stringbundle). Why does the .visuually-hidden class matter then?

Flags: needinfo?(gijskruitbosch+bugs)

(In reply to Nidhi Kumari from comment #5)

(In reply to :Gijs (he/him) from comment #3)

(In reply to Nidhi Kumari from comment #2)

Hey! I would like to work on this issue can you assign it to me?

Sure.

Are we not supposed to select any script or stringbundle?

We shouldn't, no.

If so, we can just do body:not(script, stringbundle)

No, because that would select the single body element, which doesn't exist and isn't what we want.

Also no because :not(script, stringbundle) is not a valid CSS selector (you can't have commas inside the :not()).

The current selector is: "#mainPrefPane > *:not([data-hidden-from-search]). That selects all elements that are children of #mainPrefPane that don't have a data-hidden-from-search attribute. We need to update the selector so it also doesn't select script tags and also doesn't select stringbundle tags. To do this you can add two more :not() selectors.

. Why does the .visuually-hidden class matter then?

comment #0 talks about the visually-hidden class being added to these elements by the other code in findInPage.js . We can avoid this by making sure these elements aren't included in this selector. That way, we won't bother trying to process them (a side effect of which was them being given the visually-hidden class).

Flags: needinfo?(gijskruitbosch+bugs) → needinfo?(nimbus2020b)
Attachment #9051640 - Attachment description: Bug 1534329 - Filter <script> <stringbundle> with class visually-hidden. r=Gijs → Bug 1534329 - Filter <script> <stringbundle> in the prefs' findInPage code. r=Gijs
Pushed by gijskruitbosch@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/2be7cd7b958d
Filter <script> <stringbundle> in the prefs' findInPage code. r=Gijs
Status: NEW → RESOLVED
Closed: 9 months ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 68
You need to log in before you can comment on or make changes to this bug.