Last Comment Bug 263683 - Find Bar "Highlight All" should use selection, not manipulate DOM
: Find Bar "Highlight All" should use selection, not manipulate DOM
Status: VERIFIED FIXED
:
Product: Toolkit
Classification: Components
Component: Find Toolbar (show other bugs)
: Trunk
: All All
: -- normal with 2 votes (vote)
: mozilla1.9.1a2
Assigned To: Graeme McCutcheon [:graememcc]
:
Mentors:
http://sniperontheroof.blogspot.com/
: 356031 434224 (view as bug list)
Depends on: 256773 448661 448676 451203 451204 451212 451232 451252 451286 451540
Blocks: 279022 342101 240432 251862 255941 256989 260131 273304 373371 429732
  Show dependency treegraph
 
Reported: 2004-10-09 17:38 PDT by Fareed Rizkalla
Modified: 2014-08-20 16:13 PDT (History)
29 users (show)
hskupin: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
the code (56.33 KB, image/jpeg)
2004-10-22 09:45 PDT, Fareed Rizkalla
no flags Details
rendering of the page (51.56 KB, image/jpeg)
2004-10-22 09:46 PDT, Fareed Rizkalla
no flags Details
testcase (162 bytes, text/html)
2007-12-05 11:49 PST, Daniel Holbert [:dholbert]
no flags Details
Backend Gecko changes (9.08 KB, patch)
2008-06-28 15:29 PDT, Graeme McCutcheon [:graememcc]
roc: review+
roc: superreview+
Details | Diff | Review
Findbar widget frontend changes (12.44 KB, patch)
2008-06-28 15:32 PDT, Graeme McCutcheon [:graememcc]
no flags Details | Diff | Review
Tests (4.83 KB, patch)
2008-06-28 15:34 PDT, Graeme McCutcheon [:graememcc]
no flags Details | Diff | Review
Findbar widget frontend changes (12.44 KB, patch)
2008-06-29 09:37 PDT, Graeme McCutcheon [:graememcc]
no flags Details | Diff | Review
Findbar widget frontend changes (10.99 KB, patch)
2008-06-30 09:48 PDT, Graeme McCutcheon [:graememcc]
no flags Details | Diff | Review
Findbar widget frontend changes (12.67 KB, patch)
2008-07-10 05:00 PDT, Graeme McCutcheon [:graememcc]
asaf: review+
Details | Diff | Review
Tests (10.54 KB, patch)
2008-07-10 08:07 PDT, Graeme McCutcheon [:graememcc]
no flags Details | Diff | Review
Tests (11.07 KB, patch)
2008-07-12 12:04 PDT, Graeme McCutcheon [:graememcc]
roc: review+
Details | Diff | Review
Findbar widget frontend changes (12.64 KB, patch)
2008-07-23 11:59 PDT, Graeme McCutcheon [:graememcc]
graeme.mccutcheon: review+
Details | Diff | Review
Tests (11.63 KB, patch)
2008-07-23 12:05 PDT, Graeme McCutcheon [:graememcc]
asaf: review+
Details | Diff | Review
Unified patch for checkin (33.34 KB, patch)
2008-07-23 13:23 PDT, Graeme McCutcheon [:graememcc]
graeme.mccutcheon: review+
graeme.mccutcheon: superreview+
Details | Diff | Review
To disable failing test on tinderbox (4.20 KB, patch)
2008-07-31 09:43 PDT, Graeme McCutcheon [:graememcc]
no flags Details | Diff | Review
Reftest-like mochitest for checking the rendering of the selection (11.89 KB, patch)
2009-07-02 08:02 PDT, Graeme McCutcheon [:graememcc]
roc: review+
Details | Diff | Review

Description Fareed Rizkalla 2004-10-09 17:38:41 PDT
User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.7.3) Gecko/20041004 Firefox/0.10
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.7.3) Gecko/20041004 Firefox/0.10

You see sometimes I search for things for the occurences in the page.
But then when I post it I thought it was a rendering problem, but when I tested
it in ie I found out that it gets published in the blog.

Reproducible: Always
Steps to Reproduce:
1. make a blog at blogger
2. write a blog using the wyiswyg editor, find something
3. when it marks yellow publish it

Actual Results:  
you will have a yellow marking on page

Expected Results:  
the page should appear normal and should not display the find as you type result
Comment 1 Christian :Biesinger (don't email me, ping me on IRC) 2004-10-09 18:06:54 PDT
this is a firefox-specific feature...
Comment 2 Jesse Ruderman 2004-10-09 19:19:11 PDT
Not a security hole.
Comment 3 Jeremy M. Dolan 2004-10-11 16:24:15 PDT
Testing if it affects bugzilla as well, with this comment.
The "bugzill" above should have markup cruft on it.
Comment 4 Fareed Rizkalla 2004-10-12 09:42:21 PDT
this is a test
the textarea of bugzilla is not affected
Comment 5 Fareed Rizkalla 2004-10-22 09:45:29 PDT
Created attachment 163030 [details]
the code
Comment 6 Fareed Rizkalla 2004-10-22 09:46:55 PDT
Created attachment 163031 [details]
rendering of the page
Comment 7 Gervase Markham [:gerv] 2005-09-27 01:46:14 PDT
This is an automated message, with ID "auto-resolve01".

This bug has had no comments for a long time. Statistically, we have found that
bug reports that have not been confirmed by a second user after three months are
highly unlikely to be the source of a fix to the code.

While your input is very important to us, our resources are limited and so we
are asking for your help in focussing our efforts. If you can still reproduce
this problem in the latest version of the product (see below for how to obtain a
copy) or, for feature requests, if it's not present in the latest version and
you still believe we should implement it, please visit the URL of this bug
(given at the top of this mail) and add a comment to that effect, giving more
reproduction information if you have it.

If it is not a problem any longer, you need take no action. If this bug is not
changed in any way in the next two weeks, it will be automatically resolved.
Thank you for your help in this matter.

The latest beta releases can be obtained from:
Firefox:     http://www.mozilla.org/projects/firefox/
Thunderbird: http://www.mozilla.org/products/thunderbird/releases/1.5beta1.html
Seamonkey:   http://www.mozilla.org/projects/seamonkey/
Comment 8 Adam Guthrie 2005-10-05 18:51:35 PDT
Not an issue anymore since we don't search in <textarea>s (and that's bug 252371
if anyone's interested).
Comment 9 Peter Kasting 2006-05-01 10:09:31 PDT
Actually, this still is a bug, it's just that the steps to reproduce given are poorly worded.  Rather than "find something" they should read "key in a search term and click 'highlight'".

We don't currently search in textareas, but we DO highlight in textareas.  The codepaths are separate.

Currently, findBar.js highlights by using the find service (which CAN find in textareas) and then actually mucking with the DOM to insert spans that it can color yellow.  Thus it will highlight things in textareas, and the highlights will be submitted to servers, printed, etc.

The fix for this is to redo the way the highlighting works to avoid changing the page's DOM (which would be good on general principle anyway).  Instead, the highlighting function should simply affect the DISPLAY of the page.  I don't yet know enough about the guts of all this to say how feasible this is.  I strongly suspect the highlight code would need to move out of JavaScript and into the C++ side of things.
Comment 10 Simon Bünzli 2006-06-08 05:33:43 PDT
Confirming (no dataloss).
Comment 11 Peter Kasting 2006-06-13 17:28:51 PDT
This does not block bug 252371, removing that bug from the blocks list.

Taking.  I know how this should be fixed, and have a halfway attempt at it tangled up with a bunch of find rewrite work in my checkout.  I hope to get to this at some unspecified point in the future.
Comment 12 Peter Kasting 2006-07-19 16:23:44 PDT
Changing the summary to better reflect the root cause of this bug.

Adding bug 251862 as a dependency since the speed problems there are also caused by the DOM manipulations we're doing.
Comment 13 Mano (::mano, needinfo? for any questions; not reading general bugmail) 2006-11-09 07:38:20 PST
*** Bug 356031 has been marked as a duplicate of this bug. ***
Comment 14 Peter Kasting 2007-01-29 23:18:43 PST
Not going to be getting to this one.

For reference, I was going to fix this by adding yet another selection type a la the spellchecker.  The main blocker was refactoring the find bar code to make proper use of this selection; I was going to take all the JS selection code out and hook things in on the C++ side.  Now that that stuff's all different I don't know whether it would be easier or harder to do this.
Comment 15 Stephanie Daugherty [:sdaugherty] 2007-03-09 12:08:59 PST
*** Bug 373371 has been marked as a duplicate of this bug. ***
Comment 16 Stephanie Daugherty [:sdaugherty] 2007-03-09 12:10:38 PST
373371 appears to be the same problem, but with the symptoms described differently, and there is a screenshot there of the problem occurring that may be worth note.
Comment 17 Reed Loden [:reed] (use needinfo?) 2007-07-19 23:34:31 PDT
There is a patch for this for the 1.8 branch in bug 373371.
Comment 18 Steffen Wilberg 2007-07-20 00:20:34 PDT
No, it doesn't fix this bug I'm afraid, only a minor symptom.
Comment 19 Daniel Holbert [:dholbert] 2007-12-05 11:49:11 PST
Created attachment 291715 [details]
testcase

 Expected behavior:  'abc' should be highlighted.
 Actual behavior: 'abc' is highlighted AND floats to the right.
Comment 20 Jesse Ruderman 2008-05-17 15:34:44 PDT
*** Bug 434224 has been marked as a duplicate of this bug. ***
Comment 21 Graeme McCutcheon [:graememcc] 2008-06-28 15:26:01 PDT
Some patches should follow this comment.

My original gameplan was going to be a variation on Peter Kasting's idea above - introduce a new selection type SELECTION_FIND, to contain the ranges that were found, and a display type SELECTION_HIGHLIGHT to control the yellow display of those, or indeed arbitrary ranges, which would neatly knock most of bug 256773 on the head as well.

However, in the end, this wasn't workable, SELECTION_HIGHLIGHT disappeared, and the presentation code had to be associated with SELECTION_FIND. Basically, I hit a variation of bug 209989 - the normal selection would start highlighting in yellow - but because the highlight needed to persist, there was no neat fix the way there was with that bug. Attempting to change the selection display would repaint the highlights back to the standard selection colour.

COLOURS
The highlight colour is generally #F0E020. Sometimes, the Gecko selection painting code will flip foreground and background selection colours, for maximum contrast (this generally happens on some dark greens and light blues). This does lead to the highlight occasionally being two different colours, but that is going to happen regardless what colour was picked. The alternative would be to disable the call to EnsureSufficientContrast - but that would mean we still don't correctly handle yellow highlighting on a yellow background.

Incidentally, this is why I couldn't use the existing hard-coded highlighting colour of #FFFF00 - it was triggering a reversal, causing black on yellow on white backgrounds.

In any case, if the colour needs to change, it will now be a trivial fix to nsXPLookAndFeel. Additionally, users will be able to override it at will with the ui.textHighlightBackground pref.

PERFORMANCE
I've done some thoroughly non-scientific measuring - new Date().getTime()'s inserted at various points when following the old and new code paths. Results are as follows:

Minefield first run page (search for "Minefield" - 8 matches)
Highlight all: (current code) 18ms
Highlight all: (this patch) 5ms
Unhighlight all: (current code) 6ms
Unhighlight all: (this patch) 4ms

nsGlobalWindow source code on mozilla-central mxr - search for "ns" - 1799 matches) - (Stress test suggested in bug 251862)
Highlight all: (current code) 12976ms
Highlight all: (this patch) 2326ms
Unhighlight all: (current code) 28956ms
Unhighlight all: (this patch) 1912ms

nsGlobalWindow source code on mozilla-central mxr - search for "e" - 10499 matches) - (Stress test suggested in bug 251862)
Highlight all: (current code) 137246ms
Highlight all: (this patch) 13050ms
Unhighlight all: (current code) 145295ms
Unhighlight all: (this patch) 15923ms

More improvements may be possible in the removal code, particularly if bug 339400 gets fixed. Really, removing the highlight should just be a case of getting the selection from the controller, and calling removeAllRanges(). However, as editors have independent selection controllers, and there's no easy catch-all way of determining if there are editors within the document, I'm stuck with doing a full search, to find them.

The patch just gives parity with existing functionality - I should be able to tackle some of the bugs blocked by this pretty quickly if this lands.

And now to the patches...
Comment 22 Graeme McCutcheon [:graememcc] 2008-06-28 15:29:41 PDT
Created attachment 327292 [details] [diff] [review]
Backend Gecko changes

Backend patch.

I've got showFunc=true under diff in my .hgrc, but it seems to have ignored it :-(
Comment 23 Graeme McCutcheon [:graememcc] 2008-06-28 15:32:50 PDT
Created attachment 327294 [details] [diff] [review]
Findbar widget frontend changes

Mike, requesting review from you, as Gavin's queue look's pretty full, and I remember you expressing your disdain about the way this is currently implemented in a bug somewhere.
Comment 24 Graeme McCutcheon [:graememcc] 2008-06-28 15:34:03 PDT
Created attachment 327295 [details] [diff] [review]
Tests
Comment 25 Graeme McCutcheon [:graememcc] 2008-06-28 15:56:35 PDT
Comment on attachment 327294 [details] [diff] [review]
Findbar widget frontend changes

Per request for change of reviewer from mconnor
Comment 26 Graeme McCutcheon [:graememcc] 2008-06-29 09:37:52 PDT
Created attachment 327328 [details] [diff] [review]
Findbar widget frontend changes

Oops. Typo in original.
Comment 27 :Gavin Sharp [email: gavin@gavinsharp.com] 2008-06-29 09:52:41 PDT
Comment on attachment 327328 [details] [diff] [review]
Findbar widget frontend changes

This looks awesome Graeme!

Just a couple of drive by comments: 

>diff --git a/toolkit/content/widgets/findbar.xml b/toolkit/content/widgets/findbar.xml

>+      <method name="_isEditable">

>+          while (aNode) {
>+            if (aNode instanceof Components.interfaces.nsIDOMNSEditableElement)
>+              return aNode;
>+            var temp = aNode;
>+            aNode = temp.parentNode;
>+          }

temp is unnecessary, you can just do aNode = aNode.parentNode;. Also, you should check for a non-null editor here, because all <input>s are nsIDOMNSEditableElements, but not all of them have editors (see bug 440334 comment 4).
Comment 28 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2008-06-29 15:17:45 PDT
You could probably write a reftest to check whether the rendering is correct. I believe reftests run with chrome privileges.
Comment 29 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2008-06-29 15:21:42 PDT
(In reply to comment #21)
> Basically, I
> hit a variation of bug 209989 - the normal selection would start highlighting
> in yellow - but because the highlight needed to persist, there was no neat fix
> the way there was with that bug. Attempting to change the selection display
> would repaint the highlights back to the standard selection colour.

I'm not sure what's going on here. You're saying you ran into an existing bug? Did you try to fix that bug? Can you at least file that bug with a testcase? I'd rather fix an underlying bug than work around it, even if I have to fix it myself.
Comment 30 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2008-06-29 15:23:31 PDT
Comment on attachment 327292 [details] [diff] [review]
Backend Gecko changes

The patch looks great other than that.
Comment 31 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2008-06-29 15:25:53 PDT
You know what would be really cool? If we supported selection types named by *strings*, and a CSS pseudo-element "-moz-selection(foo)" that authors could use to style those selections, so we wouldn't need to patch this again the next time someone wanted a new kind of selection.
Comment 32 Graeme McCutcheon [:graememcc] 2008-06-30 09:48:18 PDT
Created attachment 327420 [details] [diff] [review]
Findbar widget frontend changes

Comments addressed.

Gavin, are you able to do final review? Bonsai tells me you've looked over findbar code before.
Comment 33 Graeme McCutcheon [:graememcc] 2008-06-30 10:53:48 PDT
> I believe reftests run with chrome privileges.

I don't seem to have access to the gBrowser object, and I'm hitting xpconnect permission denied errors trying to get a hold of the browser, using the snippet at the end of http://developer.mozilla.org/en/docs/Code_snippets:Tabbed_browser#Getting_access_to_the_browser
Comment 34 Graeme McCutcheon [:graememcc] 2008-06-30 13:04:08 PDT
> I'm not sure what's going on here. You're saying you ran into an existing bug?
> Did you try to fix that bug? Can you at least file that bug with a testcase?
> I'd rather fix an underlying bug than work around it, even if I have to fix it
> myself.

Well, I wasn't entirely convinced the problem wasn't me and the way I was doing things!

To clarify: with a display type of SELECTION_HIGHLIGHT to control the yellow highlighting, and SELECTION_FIND acting as a container, the findbar code would call controller.setDisplaySelection(SELECTION_HIGHLIGHT) followed by controller.repaintSelection(SELECTION_FIND). However, after this, any "normal" selections made using the mouse etc would have the yellow colour of SELECTION_HIGHLIGHT rather than the standard colours of SELECTION_ON. Appending a call of controller.setDisplaySelection(SELECTION_ON) would repaint the existing highlight selections with the standard selection colours.

Comment 35 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2008-06-30 15:15:37 PDT
(In reply to comment #33)
> > I believe reftests run with chrome privileges.
> 
> I don't seem to have access to the gBrowser object, and I'm hitting xpconnect
> permission denied errors trying to get a hold of the browser, using the snippet
> at the end of
> http://developer.mozilla.org/en/docs/Code_snippets:Tabbed_browser#Getting_access_to_the_browser

Yeah you can't get the browser. However, you can get the nsISelection and add a FIND selection to some text in the page. I think.

(In reply to comment #34)
> Well, I wasn't entirely convinced the problem wasn't me and the way I was
> doing things!

:-)

> To clarify: with a display type of SELECTION_HIGHLIGHT to control the yellow
> highlighting, and SELECTION_FIND acting as a container, the findbar code would
> call controller.setDisplaySelection(SELECTION_HIGHLIGHT) followed by
> controller.repaintSelection(SELECTION_FIND). However, after this, any "normal"
> selections made using the mouse etc would have the yellow colour of
> SELECTION_HIGHLIGHT rather than the standard colours of SELECTION_ON.
> Appending
> a call of controller.setDisplaySelection(SELECTION_ON) would repaint the
> existing highlight selections with the standard selection colours.

OK, can you make a little testcase for that and file a bug on it?
Comment 36 Graeme McCutcheon [:graememcc] 2008-07-03 08:28:50 PDT
> Yeah you can't get the browser. However, you can get the nsISelection and add a
> FIND selection to some text in the page. I think.

If I can't grab the browser, to get the controller to manually get the FIND selection, window.getSelection() is the only other option, which I assume is what you're thinking of? Unfortunately, it's hardcoded to return the NORMAL selection.
http://mxr.mozilla.org/mozilla-central/source/dom/src/base/nsGlobalWindow.cpp#6187

> OK, can you make a little testcase for that and file a bug on it?
Bug 443431
Comment 37 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2008-07-03 15:13:50 PDT
I'm sure there's a way to get the selection controller for an HTML window, although I don't know off the top of my head what it is. Whatever the browser does I guess.
Comment 38 Graeme McCutcheon [:graememcc] 2008-07-10 05:00:22 PDT
Created attachment 328860 [details] [diff] [review]
Findbar widget frontend changes

Ugh. Found another typo, also fixed a comment.
Comment 39 Graeme McCutcheon [:graememcc] 2008-07-10 08:07:29 PDT
Created attachment 328891 [details] [diff] [review]
Tests

> I don't seem to have access to the gBrowser object, and I'm hitting xpconnect
> permission denied errors...

Sigh. Once again I'm an idiot. My about config prefs != the testing boxes prefs.

Reftest added. While I was at it, I rewrote the mochitests as well.
Comment 40 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2008-07-10 19:42:07 PDT
I think you might be able to get the docshell without going through gBrowser using something like

      window.QueryInterface(Ci.nsIInterfaceRequestor)
            .getInterface(Ci.nsIWebNavigation)
            .QueryInterface(Ci.nsIDocShell);

The test depends on the exact color used for the FIND selection. That would break if we start using a platform theme color. I'm not sure how to address that. You could set the ui.textHighlightBackground pref to something known, but then I'm not sure how to unset it after the reftest has run. Maybe putting it in onunload would work? Another option would be to create a CSS system color corresponding to textHighlightBackground.
Comment 41 Graeme McCutcheon [:graememcc] 2008-07-12 12:04:09 PDT
Created attachment 329244 [details] [diff] [review]
Tests
Comment 42 Graeme McCutcheon [:graememcc] 2008-07-12 12:05:48 PDT
Comment on attachment 328891 [details] [diff] [review]
Tests

Oops forgot to obsolete old patch. Apologies for bugspam.

roc: your suggestions worked beautifully.
Comment 43 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2008-07-13 15:04:27 PDT
Comment on attachment 329244 [details] [diff] [review]
Tests

Fantastic work!
Comment 44 Mano (::mano, needinfo? for any questions; not reading general bugmail) 2008-07-22 12:11:51 PDT
Comment on attachment 328860 [details] [diff] [review]
Findbar widget frontend changes


 
       <!--
+        - Gets the selection controller for the current browser 
+        -->
+      <method name="_getSelectionController">
+        <body><![CDATA[
+          // Yuck. See bug 138068.
+          var controller = this.browser.docShell.QueryInterface(Components.interfaces.nsIInterfaceRequestor)
+                                                .getInterface(Components.interfaces.nsISelectionDisplay)
+                                                .QueryInterface(Components.interfaces.nsISelectionController);

nit: too-long lines.

+          return controller;
+        ]]></body>
+      </method>
+
+      <method name="_isEditable">
+        <parameter name="aNode"/>
+        <body><![CDATA[
+          while (aNode) {
+            if (aNode instanceof Components.interfaces.nsIDOMNSEditableElement) {
+              if (aNode.editor)
+                return aNode;
+              else
+                return null;


make that |return aNode.editor ? aNode : null;|

+            }
+            aNode = aNode.parentNode;
+          }
+          return null;           
+        ]]></body>
+      </method>

the name of this method implies a boolean return value, but you're returning a dom node. Maybe rename it to getEditableNode?

       </method>
 
       <!--
         - (Un)highlights each instance of the searched word in the passed
         - window's content.
-        - @param aHighBackColor
-        -        the background color for the highlight
-        - @param aHighTextColor
-        -        the text color for the highlight, or null to make it
-        -        unhighlight
+        - @param aHighlight (boolean)
+        -        Whether to turn on highlight

..."on or off"

         - @param aWord
         -        the word to search for
         - @param aWindow
         -        the window to search in. Passing undefined will search the
         -        current content window.
         - @returns true if aWord was found
         -->
       <method name="_highlightDoc">
-        <parameter name="aHighBackColor"/>
-        <parameter name="aHighTextColor"/>
+        <parameter name="aHighlight"/>
         <parameter name="aWord"/>
         <parameter name="aWindow"/>
         <body><![CDATA[
           var win = aWindow || this.browser.contentWindow;
 
+          if (!aHighlight && !aWord)
+            return false;
+

Shouldn't this be |if (!aWord)|?


+          var controller = this._getSelectionController();
+          if (!controller) {
+            // Without the selection controller, 
+            // we are unable to (un)highlight any matches
+            textFound = false;
             return textFound;
            
just return false ;)

r=mano otherwise.
Comment 45 Mano (::mano, needinfo? for any questions; not reading general bugmail) 2008-07-22 12:20:18 PDT
Comment on attachment 329244 [details] [diff] [review]
Tests


>+
>+    function onPageShow() {
>+      gBrowser.removeEventListener("load", onPageShow, true);
>+      gFindBar.open();
>+      var search = "mozilla";
>+      gFindBar._findField.value = search;
>+      var matchCase = gFindBar.getElement("find-case-sensitive");
>+      if (matchCase.checked)
>+        matchCase.checked = false;
>+      

I prefer using doCommand here.

nit: trailing spaces
>+      gFindBar._find();
>+      var highlightButton = gFindBar.getElement("highlight");

>+      if (highlightButton.checked)
>+        highlightButton.checked = false;
>+
>+      highlightButton.click();
>+

why are both needed?


>+      ok(selection.getRangeAt(0).toString().toLowerCase() == search, "Added the correct match");
>+      highlightButton.click();
>+      ok(selection.rangeCount == 0, "Correctly removed the range");
>+
>+      var input = gBrowser.contentWindow.document.getElementById("inp");

you can use gBrowser.contentDocument.
Comment 46 Graeme McCutcheon [:graememcc] 2008-07-23 11:59:44 PDT
Created attachment 330974 [details] [diff] [review]
Findbar widget frontend changes

> nit: too-long lines.
Fixed.

> make that |return aNode.editor ? aNode : null;|
Fixed.

> the name of this method implies a boolean return value, but you're returning a
> dom node. Maybe rename it to getEditableNode?
OK.

> ..."on or off"
Done.

> Shouldn't this be |if (!aWord)|?
Yes. File under "it seemed to make sense at the time, but in fact makes no sense".

> just return false ;)
Oh dear! Fixed.

Carrying forward r=mano.
Comment 47 Graeme McCutcheon [:graememcc] 2008-07-23 12:05:19 PDT
Created attachment 330978 [details] [diff] [review]
Tests

> I prefer using doCommand here. nit: trailing spaces
OK.

> Why are both needed?
Indeed they are not. Forgot that when the highlight button is already checked, _find() will use the setHighlightTimeout method to toggle the highlight automatically. 

> you can use gBrowser.contentDocument.
And so I shall!
Comment 48 Mano (::mano, needinfo? for any questions; not reading general bugmail) 2008-07-23 12:15:57 PDT
Comment on attachment 330978 [details] [diff] [review]
Tests

r=mano
Comment 49 Graeme McCutcheon [:graememcc] 2008-07-23 13:23:57 PDT
Created attachment 330987 [details] [diff] [review]
Unified patch for checkin

Carrying forward r+sr=roc, r=mano.

My work here is done.
Comment 50 Mano (::mano, needinfo? for any questions; not reading general bugmail) 2008-07-30 13:50:31 PDT
Changeset 793aba82ef76.
Comment 51 Graeme McCutcheon [:graememcc] 2008-07-31 09:43:22 PDT
Created attachment 331844 [details] [diff] [review]
To disable failing test on tinderbox

The reftest is causing orangeness and epic build cycles on Tinderbox. The boxes don't have the right pref after all.
Comment 52 Boris Zbarsky [:bz] 2008-07-31 11:24:55 PDT
I pushed the one-line change to reftest.list as changeset 588493551f72.
Comment 53 Boris Zbarsky [:bz] 2008-07-31 11:38:14 PDT
Graeme, want to file a bug about reenabling this test?  It might depend on bug 374458 or bug 430652, I guess.  Probably mark dependent on both for now.
Comment 54 Henrik Skupin (:whimboo) 2008-08-01 18:27:00 PDT
Looks great. Verified while using the testcase (attachment 291715 [details]) with:

Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.1a2pre) Gecko/2008080103 Minefield/3.1a2pre ID:2008080103

Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.0.2pre) Gecko/2008072704 GranParadiso/3.0.2pre ID:2008072704

Tests are also checked-in - setting appropriate flag.
Comment 55 Graeme McCutcheon [:graememcc] 2009-07-02 08:02:44 PDT
Created attachment 386515 [details] [diff] [review]
Reftest-like mochitest for checking the rendering of the selection

>You could probably write a reftest to check whether the rendering is correct.

>The reftest is causing orangeness and epic build cycles on Tinderbox. 
>The boxes don't have the right pref after all.

Bug 448676 looks likes it will likely be a WONTFIX.

Anyway, at the time I wrote this, I wasn't aware of snapshotWindow/compareSnapshot which bz pointed me towards in bug 451286. This seems to be the best approach for getting a test that checks the rendering in. The patch removes the old disabled reftest from the tree.
Comment 56 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2009-07-02 19:13:39 PDT
Comment on attachment 386515 [details] [diff] [review]
Reftest-like mochitest for checking the rendering of the selection

great, thanks!
Comment 57 Graeme McCutcheon [:graememcc] 2009-07-03 16:00:42 PDT
Pushed the layout test to m-c:
http://hg.mozilla.org/mozilla-central/rev/99cf4f9414d7
Comment 58 Graeme McCutcheon [:graememcc] 2009-07-06 14:11:06 PDT
Also landed on branch:
http://hg.mozilla.org/releases/mozilla-1.9.1/rev/4fdf4d62e797
Comment 59 Michael Ryan 2009-07-12 12:31:56 PDT
Is it just me, or is there nothing shown in those changesets?
Comment 60 Henrik Skupin (:whimboo) 2009-07-13 02:55:40 PDT
No, it's not. Graeme, looks like you pushed two empty changesets?
Comment 61 Graeme McCutcheon [:graememcc] 2009-07-13 10:18:10 PDT
Odd, I see it too.

>No, it's not. Graeme, looks like you pushed two empty changesets?

...except, here's the new file correctly showing in the tree:
http://mxr.mozilla.org/mozilla-central/source/layout/generic/test/test_bug263683.html?force=1

and note the absence of the defunct 263683-1 and 263681-1-ref.html:
http://mxr.mozilla.org/mozilla-central/source/layout/reftests/bugs/
Comment 62 Daniel Holbert [:dholbert] 2009-07-13 11:24:38 PDT
Yeah -- you can see the (nonempty) changes by clicking the "raw" link at the top of the changeset page, or by running this on the command line:
m-c:   hg diff -r 01e14fa57337 -r 99cf4f9414d7
1.9.1: hg diff -r c21d4f5fb12a -r 4fdf4d62e797

Looks like this is just a bug in the Mercurial web interface.
Comment 63 Daniel Holbert [:dholbert] 2009-07-13 11:40:18 PDT
I filed Bug 503910 to track the above-mentioned bug in the hg web interface.
Comment 64 Daniel Holbert [:dholbert] 2009-07-13 13:23:51 PDT
(hg web interface issue has now been resolved -- the changesets in comment 57 & comment 58 no longer look empty)

Note You need to log in before you can comment on or make changes to this bug.