305381 - Find doesn't ignore elements styled as display:none (finds hidden/invisible elements)

Status: RESOLVED WORKSFORME

(Core :: Find Backend, defect, P5)

Description

[u49640]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8b4) Gecko/20050820 Firefox/1.0+
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8b4) Gecko/20050820 Firefox/1.0+

Sometimes the Findfast search doesnt work in the Help viewer (unfortunatly it
didnt happen all the time, but sometimes)

Reproducible: Sometimes

Steps to Reproduce:
1.Open The keyboard shortcut help page
2. open findfast
3. enter "alt"
--> the first text with "alt" is selectet"
enter "+"
nothing is found, but there are a lot of "Alt+" strings in there

or

search for "ctrl+" works, but "ctrl+l" doesnt (sometimes this one did find
something)



Expected Results:  
a search for "alt+" should work and "ctrl+l" should work too

Comment 1

[u49640]

(changing it to Find toolbar, i think it should be over there)

Comment 2

[Ria Klaassen (not reading all bugmail)]

Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9a1) Gecko/20050821
Firefox/1.6a1 ID:2005082101

Yes, I see the same. 

Comment 3

[Martijn Wargers (dead)]

It's this code:
<span class="win">

            <kbd class="noMac">
Alt  </kbd>
  <kbd class="mac">
Opt  </kbd>
+  <kbd>
Right Arrow  </kbd>
  <br/>

            <kbd>
Shift  </kbd>
+  <kbd>
Backspace  </kbd>

        </span>

So on windows, the class="mac" is invisible.
But that's not something that the find function can 'see'.

Comment 4

[Steffen Wilberg]

Indeed. It finds "AltOpt+".

http://lxr.mozilla.org/seamonkey/source/browser/locales/en-US/chrome/help/platformStrings.dtd#31
http://lxr.mozilla.org/seamonkey/source/toolkit/components/help/content/platformClasses.css#38

Comment 5

[Jeff Walden [:Waldo]]

This also would affect searches for "option" or "preference", as they're defined
similarly to how shortcut keys are defined.

Note the help viewer could probably work around this by removing elements which
aren't displayed in the current help doc using a script like the one below (may
not work, I've never used tree walkers before):

<script type="application/x-javascript">

var searchRegexp = new RegExp( "\b(" +
#ifdef XP_WIN
                   "noWin|mac|unix" +
#else
#ifdef XP_MACOSX
                   "noMac|win|unix" +
#else
#ifdef XP_OS2
                   "noWin|mac|unix" +
#else
                   "noUnix|win|mac" +
#endif
#endif
#endif
                   ")\b" );

var filter = {
  acceptNode: function(node) {
    if (node.getAttribute("class").search(searchRegexp) < 0)
      return NodeFilter.FILTER_REJECT;
    else
      return NodeFilter.FILTER_ACCEPT;
  }
};

var elemsToHide = document.createNodeIterator(document.documentElement,
                                              NodeFilter.SHOW_ELEMENT,
                                              filter,
                                              true);

var nextNode;
while(elemsToHide.nextNode()) {
  nextNode = elemsToHide.currentNode;
  nextNode.parentNode.removeChild(nextNode);
}

</script>

However, this errant behavior is an edge case (based on the number of times
someone's reported the bug), and I'm not sure I would even make a case for the
workaround in trunk code, let alone branch code.

Comment 6

[Martijn Wargers (dead)]

Attachment: testcase

Comment 7

[Martijn Wargers (dead)]

Ok, IE6 acts the same as Mozilla, so I don't think that is a bug.
So the help viewer needs to get fixed.

(In reply to comment #5)
createNodeIterator is not implemented in Mozilla.
Something like this is working:
<script>
window.onload = function() {
var nodes = document.getElementsByTagName('body')[0].getElementsByTagName('*');
var nodes_length = nodes.length;
for (var i=nodes_length-1; i >= 0; i--) {
 if (window.getComputedStyle(nodes[i], '').getPropertyValue('display') == 'none')
    nodes[i].parentNode.removeChild(nodes[i]);
}
}
</script>

But it makes things slower.
I think it is best to not insert those nodes when not necessary.
Isn't it possible to use ifdefs in platformStrings.dtd instead of in
platformStrings.css?

Comment 8

[u49640]

(In reply to comment #7)
> Ok, IE6 acts the same as Mozilla, 

We can do better than IE

> so I don't think that is a bug.

No, it should find the stuff on the screen. (i dont think that anyone wants to
find hidden Text. its hidden for a reason)

or is there any reason why hidden Text is included in the search?
(since there is no (normal) way of showing the search result its pretty much not
usefull to search for it)

Comment 9

[Martijn Wargers (dead)]

Mozilla doesn't find stuff that is invisible (or display:none), so that's not
the issue here (the summary is probably not a good one here).
The issue is that a word that is separated by a display:none element, isn't found.
Now, you could call that a bug (although it might not be).
But we probably need a separate bug, because the help viewer issue can be fixed
without this issue.

Comment 10

[Steffen Wilberg]

> Isn't it possible to use ifdefs in platformStrings.dtd instead of in
> platformStrings.css?
#ifdefs are verboten in locale files, because langpacks ought to be
cross-platform. And we use the platform classes in help content as well, just
look at
http://lxr.mozilla.org/seamonkey/source/browser/locales/en-US/chrome/help/shortcuts.xhtml#53

IMHO fixing the help viewer would only workaround this find toolbar bug.

Comment 11

[Martijn Wargers (dead)]

Attachment: patch

This does what we want here, but it isn't any good, because it also ignore
visibility:hidden and that should not happen.

Comment 12

[Martijn Wargers (dead)]

Attachment: testcase2

With the patch, it doesn't skip the first instances of "word" in this testcase.

Comment 13

[Steffen Wilberg]

> This does what we want here, but it isn't any good, because it also ignore
> visibility:hidden and that should not happen.
Why shouldn't it? IsVisibleNode() was introduced with rev. 1.15 of nsFind.cpp,
checkin comment was "Bug 175046, bug 172991, bug 166471. Make sure typeaheadfind
and regular find don't find comment nodes, display:none, visibility:hidden or
visibility:collapsed nodes".

Comment 15

[Martijn Wargers (dead)]

(In reply to comment #13)
> > This does what we want here, but it isn't any good, because it also ignore
> > visibility:hidden and that should not happen.
> Why shouldn't it? 

You're right, I don't really know what I was thinking here.

Comment 16

[Flaky Jake]

Also happens on GNU/Linux.

Comment 17

[u49640]

-> All/All

Comment 19

[Vladan Djeric (:vladan)]

I just hit this on http://taras.glek.net/blog/2013/01/04/snappy-2012-summary/ by searching for "bug 699820"

Is this considered a low-priority issue?

Comment 22

[BugBot [:suhaib / :marco/ :calixte]]

The bug assignee didn't login in Bugzilla in the last months and this bug has severity 'major'.
:enndeakin, could you have a look please?
For more information, please visit auto_nag documentation.

Comment 23

[Emilio Cobos Álvarez (:emilio)]

Pretty sure this works now: https://searchfox.org/mozilla-central/rev/7b9d23ece4835bf355e5195f30fef942d376a1c7/toolkit/components/find/nsFind.cpp#182-187