256990 - highlight breaks pages that style <span> elements

Status: VERIFIED FIXED

(Toolkit :: Find Toolbar, defect)

Description

[Hixie (not reading bugmail)]

STEPS TO REPRODUCE
   1. Click the URL link above.
   2. Press "/".
   3. Press the space bar.
   4. Press "highlight.

ACTUAL RESULTS
   Suddenly page is full of red boxes.

EXPECTED RESULTS
   A few yellow highlights for the spaces, and that's all.

Comment 1

[Peter van der Woude [:Peter6]]

Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.7.2) Gecko/20040826
Firefox/0.9.1+

WFM

Comment 2

[Michael Clackler]

Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.7) Gecko/20040802
Firefox/0.9.1+

WFM also

Comment 3

[Phil Ringnalda (:philor)]

Hixie: you've got the testcase URL from bug 256989 (breaking form fields) in
this one too: do you remember what you intended, to have it breaking pages with
styled <span>s?

Comment 4

[:aceman]

Can reproduce with 0.10.1 e.g. on
http://www.mozilla.org/products/firefox/releases/0.10.html. Search for 'FA' and
press highlight.

Comment 5

[Phil Ringnalda (:philor)]

*** Bug 270809 has been marked as a duplicate of this bug. ***

Comment 6

[Phil Ringnalda (:philor)]

*** Bug 267676 has been marked as a duplicate of this bug. ***

Comment 7

[Phil Ringnalda (:philor)]

Changing URL to something that breaks the right way: search/highlight "t" and
Projects in the nav menu becomes
Projec
t
s
then unhighlight and it remains as
Project
s
(which is probably a separate reflow bug).

Comment 8

[Ben Basson]

Attachment: Minimised Testcase

This is really two bugs:

1. If Firefox is adding <span> elements on the assumption
   that they will be visible, it should explicity specify
   this with style="display: inline;". There are other
   desirable properties we can set too. 

2. Firefox doesn't clean up after itself. I don't think
   there are page reflow bugs here, it seems that the
   <span> elements simply aren't removed after use. This
   should probably be filed as a new bug.

This should be an easy fix.

Comment 9

[Ben Basson]

Attachment: Patch v0.1

This fixes the bug, but probably needs better testing than I've given it. I
found three problems with the existing code that caused things to break when
inserting the <span> elements:

1. FAYT doesn't specify "padding: 0", meaning that CSS padding for
   span elements was respected.

2. FAYT doesn't specify "display: inline", meaning that "display: none"
   for span elements was respected. This also had another side-effect
   of respecting "display: block", leading to other aesthetically nasty
   results.

3. When the span was removed, text nodes were not joined back togther.
   I've fixed this by calling parent.normalize(), which is necessary
   to remove a further display bug that I found.

Fixing this exposes another bug, which I'll file dependent on this one being
fixed.

Comment 10

[Ben Basson]

(In reply to comment #9)
> Fixing this exposes another bug, which I'll file dependent on this one being
> fixed.

The bug is reproducable without this fix, therefore is not dependent.

Comment 11

[Ben Basson]

Sorry for bugspam, I filed Bug 302933 for a case where highlighting wasn't
removed as per comment 8 part 2. This doesn't appear to be the norm (in nearly
all cases, the span is removed correctly).

Comment 12

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Until it stops messing with the DOM at all and starts using the selection code
it will always break some pages.

Comment 13

[Ben Basson]

You're right, but this fix would work as a stopgap measure until then which
should deal with the more common problems that I can spot.

Comment 14

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

You should make sure there's a bug open on that before closing this one.  (I
remember commenting on one, but I can't find it, so it may have been resolved
for some reason.)

Comment 15

[Ben Basson]

David, you filed bug 256773 for that ;) This bug should probably depend on it,
since your suggestion is the true fix. Marking as such. 

Looks like my bug 302933 is a dupe of bug 273304 (which I somehow missed when
searching). I'll go sort that out, too.

Comment 16

[Ben Basson]

Comment on attachment 191202 [details] [diff] [review]
Patch v0.1

Requestiong approval - this is low risk and fixes a handful of edge-cases where
the Findbar highlighter either fails to work or breaks the page.

Comment 17

[:Gavin Sharp [email: [e-mail]]]

Trunk:
/cvsroot/mozilla/toolkit/components/typeaheadfind/content/findBar.js;
new revision: 1.22;

1.8 Branch:
/cvsroot/mozilla/toolkit/components/typeaheadfind/content/findBar.js;
new revision: 1.21.2.1;

Comment 18

[Greg K.]

Verified fixed using Win FF 1.5.