Find Bar "Highlight All" should use selection, not manipulate DOM

VERIFIED FIXED in mozilla1.9.1a2

Status

()

Toolkit
Find Toolbar
VERIFIED FIXED
13 years ago
3 years ago

People

(Reporter: Fareed Rizkalla, Assigned: graememcc)

Tracking

(Depends on: 1 bug, Blocks: 2 bugs)

Trunk
mozilla1.9.1a2
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

(URL)

Attachments

(9 attachments, 7 obsolete attachments)

56.33 KB, image/jpeg
Details
51.56 KB, image/jpeg
Details
162 bytes, text/html
Details
9.08 KB, patch
roc
: review+
Details | Diff | Splinter Review
12.64 KB, patch
graememcc
: review+
Details | Diff | Splinter Review
11.63 KB, patch
mano
: review+
Details | Diff | Splinter Review
33.34 KB, patch
graememcc
: review+
graememcc
: superreview+
Details | Diff | Splinter Review
4.20 KB, patch
Details | Diff | Splinter Review
11.89 KB, patch
roc
: review+
Details | Diff | Splinter Review
(Reporter)

Description

13 years ago
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
this is a firefox-specific feature...
Assignee: general → firefox
Component: Browser-General → Find Toolbar / FastFind
Product: Browser → Firefox
QA Contact: general
Version: Trunk → 1.0 Branch

Comment 2

13 years ago
Not a security hole.
Group: security
Severity: blocker → major
Keywords: dataloss
Summary: Using the find as you type feature, messes things up → Firefox's Find highlighting gets submitted to server (Blogger/Midas)

Comment 3

13 years ago
Testing if it affects bugzilla as well, with this comment.
The "bugzill" above should have markup cruft on it.
(Reporter)

Comment 4

13 years ago
this is a test
the textarea of bugzilla is not affected
(Reporter)

Comment 5

13 years ago
Created attachment 163030 [details]
the code
(Reporter)

Comment 6

13 years ago
Created attachment 163031 [details]
rendering of the page
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

12 years ago
Not an issue anymore since we don't search in <textarea>s (and that's bug 252371
if anyone's interested).
Status: UNCONFIRMED → RESOLVED
Last Resolved: 12 years ago
Resolution: --- → INVALID

Updated

11 years ago
Blocks: 252371

Comment 9

11 years ago
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.
Status: RESOLVED → UNCONFIRMED
Resolution: INVALID → ---

Comment 10

11 years ago
Confirming (no dataloss).
Severity: major → normal
Status: UNCONFIRMED → NEW
Ever confirmed: true
Keywords: dataloss
Version: 1.0 Branch → unspecified

Comment 11

11 years ago
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.
Assignee: firefox → pkasting
No longer blocks: 252371

Updated

11 years ago
Blocks: 342101
QA Contact: fast.find

Updated

11 years ago
Blocks: 256989

Comment 12

11 years ago
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.
Blocks: 251862
Summary: Firefox's Find highlighting gets submitted to server (Blogger/Midas) → Find Bar Highlight functionality should use selection, not manipulate DOM

Updated

11 years ago
Blocks: 279022
Blocks: 356031
*** Bug 356031 has been marked as a duplicate of this bug. ***
No longer blocks: 356031

Comment 14

11 years ago
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.
Assignee: pkasting → nobody

Updated

10 years ago
Depends on: 256773
Duplicate of this bug: 373371
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.
OS: Windows XP → All
Hardware: PC → All
Version: unspecified → Trunk
There is a patch for this for the 1.8 branch in bug 373371.

Updated

10 years ago
Blocks: 373371

Comment 18

10 years ago
No, it doesn't fix this bug I'm afraid, only a minor symptom.
Created attachment 291715 [details]
testcase

 Expected behavior:  'abc' should be highlighted.
 Actual behavior: 'abc' is highlighted AND floats to the right.

Updated

9 years ago
Summary: Find Bar Highlight functionality should use selection, not manipulate DOM → Find Bar "Highlight All" should use selection, not manipulate DOM

Updated

9 years ago
Duplicate of this bug: 434224
(Assignee)

Comment 21

9 years ago
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...
Assignee: nobody → graememcc_firefox
(Assignee)

Comment 22

9 years ago
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 :-(
Attachment #327292 - Flags: superreview?(roc)
Attachment #327292 - Flags: review?(roc)
(Assignee)

Comment 23

9 years ago
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.
Attachment #327294 - Flags: review?(mconnor)
(Assignee)

Comment 24

9 years ago
Created attachment 327295 [details] [diff] [review]
Tests
(Assignee)

Updated

9 years ago
Status: NEW → ASSIGNED
(Assignee)

Comment 25

9 years ago
Comment on attachment 327294 [details] [diff] [review]
Findbar widget frontend changes

Per request for change of reviewer from mconnor
Attachment #327294 - Flags: review?(mconnor) → review?(enndeakin)
(Assignee)

Comment 26

9 years ago
Created attachment 327328 [details] [diff] [review]
Findbar widget frontend changes

Oops. Typo in original.
Attachment #327294 - Attachment is obsolete: true
Attachment #327328 - Flags: review?(enndeakin)
Attachment #327294 - Flags: review?(enndeakin)
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).
You could probably write a reftest to check whether the rendering is correct. I believe reftests run with chrome privileges.
(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 on attachment 327292 [details] [diff] [review]
Backend Gecko changes

The patch looks great other than that.
Attachment #327292 - Flags: superreview?(roc)
Attachment #327292 - Flags: superreview+
Attachment #327292 - Flags: review?(roc)
Attachment #327292 - Flags: review+
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.
(Assignee)

Comment 32

9 years ago
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.
Attachment #327328 - Attachment is obsolete: true
Attachment #327420 - Flags: review?(gavin.sharp)
Attachment #327328 - Flags: review?(enndeakin)
(Assignee)

Comment 33

9 years ago
> 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
(Assignee)

Comment 34

9 years ago
> 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.

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

Comment 36

9 years ago
> 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
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.
(Assignee)

Updated

9 years ago
Attachment #327420 - Flags: review?(gavin.sharp) → review?(mano)
(Assignee)

Comment 38

9 years ago
Created attachment 328860 [details] [diff] [review]
Findbar widget frontend changes

Ugh. Found another typo, also fixed a comment.
Attachment #327420 - Attachment is obsolete: true
Attachment #328860 - Flags: review?(mano)
Attachment #327420 - Flags: review?(mano)
(Assignee)

Comment 39

9 years ago
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.
Attachment #327295 - Attachment is obsolete: true
Attachment #328891 - Flags: review?(roc)
Attachment #328891 - Flags: review?(mano)
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.
(Assignee)

Comment 41

9 years ago
Created attachment 329244 [details] [diff] [review]
Tests
Attachment #329244 - Flags: review?(roc)
Attachment #329244 - Flags: review?(mano)
(Assignee)

Comment 42

9 years ago
Comment on attachment 328891 [details] [diff] [review]
Tests

Oops forgot to obsolete old patch. Apologies for bugspam.

roc: your suggestions worked beautifully.
Attachment #328891 - Attachment is obsolete: true
Attachment #328891 - Flags: review?(roc)
Attachment #328891 - Flags: review?(mano)
Comment on attachment 329244 [details] [diff] [review]
Tests

Fantastic work!
Attachment #329244 - Flags: review?(roc) → review+
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.
Attachment #328860 - Flags: review?(mano) → review+
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.
(Assignee)

Comment 46

9 years ago
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.
Attachment #328860 - Attachment is obsolete: true
Attachment #330974 - Flags: review+
(Assignee)

Comment 47

9 years ago
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!
Attachment #329244 - Attachment is obsolete: true
Attachment #330978 - Flags: review?(mano)
Attachment #329244 - Flags: review?(mano)
Comment on attachment 330978 [details] [diff] [review]
Tests

r=mano
Attachment #330978 - Flags: review?(mano) → review+
(Assignee)

Comment 49

9 years ago
Created attachment 330987 [details] [diff] [review]
Unified patch for checkin

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

My work here is done.
Attachment #330987 - Flags: superreview+
Attachment #330987 - Flags: review+
(Assignee)

Updated

9 years ago
Keywords: checkin-needed
Blocks: 429732
Changeset 793aba82ef76.
Status: ASSIGNED → RESOLVED
Last Resolved: 12 years ago9 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Product: Firefox → Toolkit
(Assignee)

Comment 51

9 years ago
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.
I pushed the one-line change to reftest.list as changeset 588493551f72.
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.
Depends on: 448676
Depends on: 448661
(Assignee)

Updated

9 years ago
Target Milestone: --- → mozilla1.9.1a2
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.
Status: RESOLVED → VERIFIED
Flags: in-testsuite+

Updated

9 years ago
Depends on: 451203

Updated

9 years ago
Depends on: 451204

Updated

9 years ago
Depends on: 451212

Updated

9 years ago
Depends on: 451232

Updated

9 years ago
Depends on: 451252
Blocks: 255941

Updated

9 years ago
Blocks: 260131

Updated

9 years ago
Blocks: 273304
(Assignee)

Updated

9 years ago
Depends on: 451540

Updated

9 years ago
Blocks: 240432
(Assignee)

Updated

9 years ago
Depends on: 451286

Updated

9 years ago
Blocks: 455411
No longer blocks: 455411
(Assignee)

Comment 55

8 years ago
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.
Attachment #386515 - Flags: review?(roc)
Comment on attachment 386515 [details] [diff] [review]
Reftest-like mochitest for checking the rendering of the selection

great, thanks!
Attachment #386515 - Flags: review?(roc) → review+
(Assignee)

Comment 57

8 years ago
Pushed the layout test to m-c:
http://hg.mozilla.org/mozilla-central/rev/99cf4f9414d7
(Assignee)

Comment 58

8 years ago
Also landed on branch:
http://hg.mozilla.org/releases/mozilla-1.9.1/rev/4fdf4d62e797

Comment 59

8 years ago
Is it just me, or is there nothing shown in those changesets?
No, it's not. Graeme, looks like you pushed two empty changesets?
(Assignee)

Comment 61

8 years ago
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/
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.
I filed Bug 503910 to track the above-mentioned bug in the hg web interface.
(hg web interface issue has now been resolved -- the changesets in comment 57 & comment 58 no longer look empty)
You need to log in before you can comment on or make changes to this bug.