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

Status: RESOLVED FIXED

(Firefox :: Settings UI, defect, P3)

Description

[Ian Moody [:Kwan] (UTC+1)]

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

Comment 1

[:Gijs (he/him)]

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.

Comment 2

[Nidhi Kumari]

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

Comment 3

[:Gijs (he/him)]

(In reply to Nidhi Kumari from comment #2)

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

Sure.

Comment 4

[Nidhi Kumari]

Attachment: Bug 1534329 - Filter <script> <stringbundle> in the prefs' findInPage code. r=Gijs

Comment 5

[Nidhi Kumari]

(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?

Comment 6

[:Gijs (he/him)]

(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).

Comment 7

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/2be7cd7b958d
Filter <script> <stringbundle> in the prefs' findInPage code. r=Gijs

Comment 8

[Cristina Coroiu [:ccoroiu]]

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