Closed Bug 879382 Opened 6 years ago Closed 6 years ago

Create a mozmill test for finding text in a page

Categories

(Mozilla QA Graveyard :: Mozmill Tests, defect, P2)

x86
Windows 8.1
defect

Tracking

(firefox30 fixed)

RESOLVED FIXED
Tracking Status
firefox30 --- fixed

People

(Reporter: andrei, Assigned: danisielm)

References

Details

(Whiteboard: [metro])

Attachments

(1 file, 13 obsolete files)

Cross referencing bug 831941

Write a mozmill test for Firefox Metro, to test the search feature.
Depends on: 879365
Attached patch patch v1 (obsolete) — Splinter Review
Basic implementation of testing the UI for Find in Page in metro.

This is still a work in progress.

We still need to:
- use touch events // bug 880426
- reference UI elements abstracted in a lib // bug 880417
- some cleaning up
Attachment #759389 - Flags: feedback?(hskupin)
Attachment #759389 - Flags: feedback?(andreea.matei)
Depends on: 880417, 880426
Comment on attachment 759389 [details] [diff] [review]
patch v1

Review of attachment 759389 [details] [diff] [review]:
-----------------------------------------------------------------

Generally it looks fine. But a lot will have to be changed, once the ui map exists.

::: tests/metro/testFindInPage.js
@@ +1,1 @@
> +/* This Source Code Form is subject to the terms of the Mozilla Public

I would put this file into a subfolder which serves as category of the test. So we can group them similarly to the other testruns.

@@ +21,5 @@
> +  controller.open(TEST_DATA);
> +  controller.waitForPageLoad();
> +
> +  // Open search UI
> +  win.keypress("F", { accelKey: true });

For the final test we need the localized key.

@@ +23,5 @@
> +
> +  // Open search UI
> +  win.keypress("F", { accelKey: true });
> +  var searchContainer = new elementslib.ID(controller.window.document, "content-navigator");
> +  searchContainer.waitForElement();

This is chrome, so it usually be present all the time. Why do we need a waitForElement() call here?

@@ +26,5 @@
> +  var searchContainer = new elementslib.ID(controller.window.document, "content-navigator");
> +  searchContainer.waitForElement();
> +
> +  // Get references to all the buttons
> +  var collector = new nodeCollector(searchContainer.getNode());

Finally once the ui map is up, this can be done with a double call to nodeCollector. First get the XBL widget and then set the new root.

@@ +39,5 @@
> +  // Check that the correct word has been highlighted
> +  var selectedText;
> +  assert.waitFor(function() {
> +    selectedText = controller.tabs.activeTabWindow.getSelection();
> +    return selectedText.toString().toLowerCase() === SEARCH_TEXT;

I would wait for the offset here to change and check the text later via expect.equal().

@@ +51,5 @@
> +  assert.waitFor(function() {
> +    selectedText = controller.tabs.activeTabWindow.getSelection();
> +    var secondOffset = selectedText.getRangeAt(0).startOffset;
> +    return selectedText.toString().toLowerCase() === SEARCH_TEXT &&
> +           secondOffset !== firstOffset;

Same here for the offset. Given that the text should be the same, lets do a sanity check after the loop.
Attachment #759389 - Flags: feedback?(hskupin)
Attachment #759389 - Flags: feedback?(andreea.matei)
Attachment #759389 - Flags: feedback+
Depends on: 882055
Attached patch patch v2 (obsolete) — Splinter Review
Moved into a folder, added manifest files, changed checks according to previous comment.

We can't have a localized shortcut yet ( bug 882074 )
Attachment #759389 - Attachment is obsolete: true
No longer depends on: 880426
Landed a patch on inbound that incorrectly references this bug:
  https://hg.mozilla.org/integration/mozilla-inbound/rev/14503e9ec043

That patch should reference bug 879392
https://hg.mozilla.org/mozilla-central/rev/14503e9ec043
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
This has been incorrectly closed.
See comment 4 for reference.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Status: REOPENED → ASSIGNED
(In reply to Andrei Eftimie from comment #6)
> This has been incorrectly closed.
> See comment 4 for reference.

When you make such a claim, you should at least check to cc the person. I have now marked the other bug as fixed given the changeset from comment 5.
(In reply to Andrei Eftimie from comment #6)
> This has been incorrectly closed.
> See comment 4 for reference.

The bug marking tool uses the bug number from the commit message; sorry for the churn.
Priority: -- → P1
Depends on: 880426
Depends on: 908076
Attached patch 3.patch (obsolete) — Splinter Review
We might still want to have this use the UI map.
But besides that it should be ready to go.
Attachment #761377 - Attachment is obsolete: true
Attachment #795478 - Flags: review?(andreea.matei)
I would like to see at least one of the metro tests landed, so we can start with the metro testrun implementation.
Priority: P1 → P2
Comment on attachment 795478 [details] [diff] [review]
3.patch

Review of attachment 795478 [details] [diff] [review]:
-----------------------------------------------------------------

Removing the flag until we get the UI library finished, as discussed in Ask an expert meeting.
Attachment #795478 - Flags: review?(andreea.matei)
Blocks: 922197
Attached patch 4.patch (obsolete) — Splinter Review
Updated paths.

Lets get this in to be able to run a metro functional testrun.
I was able to run a functional metro testrun with this patch.

It failed when trying to send the report, but that looks like a dashboard issue.
Attachment #795478 - Attachment is obsolete: true
Attachment #8337675 - Flags: review?(andreea.matei)
Comment on attachment 8337675 [details] [diff] [review]
4.patch

Review of attachment 8337675 [details] [diff] [review]:
-----------------------------------------------------------------

::: metrofirefox/tests/functional/testFindInPage/testFindInPage.js
@@ +33,5 @@
> +
> +  // Get references to all the buttons
> +  var previous = new findElement.ID(controller.window.document, "findbar-previous-button");
> +  var next = new findElement.ID(controller.window.document, "findbar-next-button");
> +  var close = new findElement.ID(controller.window.document, "findbar-close-button");

All this should really end-up in the library! We don't want to define any of the elements in our test. If that is dependent on Andreeas work, we should get it updated with her patch.

@@ +47,5 @@
> +    return selectedText.toString().toLowerCase() === SEARCH_TEXT;
> +  }, "Text has been selected");
> +
> +  // Remember position of the first selection
> +  var firstOffset = selectedText.getRangeAt(0).startOffset;

I would call it firstSelection.

@@ +73,5 @@
> +  // Close the find toolbar and check it has closed
> +  close.tap();
> +  assert.waitFor(function() {
> +    return !findBar.getNode().isActive;
> +  }, "Find in Page toolbar has been closed");

I don't see that we force this in a teardown method.

@@ +74,5 @@
> +  close.tap();
> +  assert.waitFor(function() {
> +    return !findBar.getNode().isActive;
> +  }, "Find in Page toolbar has been closed");
> +};

I miss a reference to a moztrap test. Lets not get this in without the meta info if one exist.
Attachment #8337675 - Flags: review?(andreea.matei) → review-
Assignee: andrei.eftimie → daniel.gherasim
Attached patch 5.patch (obsolete) — Splinter Review
I have updated the patch according the last review. Some other changes needed to be done as the metro shared module changed lately.

Regarding the moztrap test, I didn't find ATM any related test on moztrap that does the same steps.
Attachment #8337675 - Attachment is obsolete: true
Attachment #8369464 - Flags: review?(andrei.eftimie)
Attachment #8369464 - Flags: review?(andreea.matei)
Comment on attachment 8369464 [details] [diff] [review]
5.patch

Review of attachment 8369464 [details] [diff] [review]:
-----------------------------------------------------------------

We can still remove some unused stuff from this test, and make some small improvements.

Otherwise it looks good.

::: metrofirefox/tests/functional/testFindInPage/testFindInPage.js
@@ +4,5 @@
> +
> +"use strict";
> +
> +var { assert } = require("../../../../lib/assertions");
> +var { nodeCollector } = require("../../../../lib/dom-utils");

This is not used anymore.
We can remove this line.

@@ +6,5 @@
> +
> +var { assert } = require("../../../../lib/assertions");
> +var { nodeCollector } = require("../../../../lib/dom-utils");
> +var toolbars = require("../../../lib/ui/toolbars");
> +var utils = require("../../../../firefox/lib/utils");

Same here. I don't see we're using utils anymore.

@@ +14,5 @@
> +const SEARCH_TEXT = "community";
> +
> +var setupModule = function(aModule) {
> +  aModule.controller = mozmill.getBrowserController();
> +  aModule.win = new findElement.MozMillElement("Elem", aModule.controller.window);

This is in the library, so its not getting used here anymore.

@@ +15,5 @@
> +
> +var setupModule = function(aModule) {
> +  aModule.controller = mozmill.getBrowserController();
> +  aModule.win = new findElement.MozMillElement("Elem", aModule.controller.window);
> +  aModule.comparator = Ci.nsIDOMRange.START_TO_START;

This is not used.

@@ +17,5 @@
> +  aModule.controller = mozmill.getBrowserController();
> +  aModule.win = new findElement.MozMillElement("Elem", aModule.controller.window);
> +  aModule.comparator = Ci.nsIDOMRange.START_TO_START;
> +
> +  aModule.toolbar = new toolbars.ToolBar(aModule.controller);

nit: we should be consistent here and name it toolBar

@@ +37,5 @@
> +  // Open the findbar
> +  findBar.open("shortcut");
> +
> +  // Get references to all the buttons
> +  var previous = findBar.getElement({type: "previousButton"});

We could name this previousButton, it will make code reading better:
eg. previousButton.tap();

Also applies to the other buttons.

@@ +59,5 @@
> +  // Check that the next occurence has been highlighted
> +  next.tap();
> +  assert.waitFor(function() {
> +    selectedText = controller.tabs.activeTabWindow.getSelection();
> +   var secondSelection = selectedText.getRangeAt(0).startOffset;

nit: alignment
Attachment #8369464 - Flags: review?(andrei.eftimie)
Attachment #8369464 - Flags: review?(andreea.matei)
Attachment #8369464 - Flags: review-
Attached patch 5.1.patch (obsolete) — Splinter Review
Thanks a lot,

Updated the patch with the latest modifications.
Attachment #8369464 - Attachment is obsolete: true
Attachment #8371325 - Flags: review?(andrei.eftimie)
Attachment #8371325 - Flags: review?(andreea.matei)
Comment on attachment 8371325 [details] [diff] [review]
5.1.patch

Review of attachment 8371325 [details] [diff] [review]:
-----------------------------------------------------------------

Small updates, otherwise looks good.

::: metrofirefox/tests/functional/testFindInPage/testFindInPage.js
@@ +8,5 @@
> +var toolbars = require("../../../lib/ui/toolbars");
> +
> +const BASE_URL = collector.addHttpResource("../../../../data/");
> +const TEST_DATA = BASE_URL + "layout/mozilla_community.html";
> +const SEARCH_TEXT = "community";

Add a blank line between the [BASE_URL, TEST_DATA] block and SEARCH_TEXT.

@@ +23,5 @@
> +  catch(e) {
> +  }
> +}
> +
> +var testFindInPage = function() {

js doc missing.
Attachment #8371325 - Flags: review?(andrei.eftimie)
Attachment #8371325 - Flags: review?(andreea.matei)
Attachment #8371325 - Flags: review-
Attached patch 5.2.patch (obsolete) — Splinter Review
Thanks for the tips,
Updated the patch.
Attachment #8371325 - Attachment is obsolete: true
Attachment #8371351 - Flags: review?(andrei.eftimie)
Attachment #8371351 - Flags: review?(andreea.matei)
Comment on attachment 8371351 [details] [diff] [review]
5.2.patch

Review of attachment 8371351 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good to me.

Adding the r? flag for Henrik since this is a new test.
Attachment #8371351 - Flags: review?(hskupin)
Attachment #8371351 - Flags: review?(andrei.eftimie)
Attachment #8371351 - Flags: review?(andreea.matei)
Attachment #8371351 - Flags: review+
Comment on attachment 8371351 [details] [diff] [review]
5.2.patch

Review of attachment 8371351 [details] [diff] [review]:
-----------------------------------------------------------------

::: metrofirefox/tests/functional/testFindInPage/testFindInPage.js
@@ +11,5 @@
> +const TEST_DATA = BASE_URL + "layout/mozilla_community.html";
> +
> +const SEARCH_TEXT = "community";
> +
> +var setupModule = function(aModule) {

No var declarations please.

@@ +21,5 @@
> +  try {
> +    toolBar.findBar.close();
> +  }
> +  catch(e) {
> +  }

Why that? In a teardown module we never want to call methods which exercise ui paths. Otherwise we might need a force flag.

@@ +43,5 @@
> +  findBarTextbox.sendKeys(SEARCH_TEXT);
> +
> +  // Check that the correct word has been highlighted
> +  var selectedText;
> +  assert.waitFor(function() {

nit: blank between function and opening bracket

@@ +45,5 @@
> +  // Check that the correct word has been highlighted
> +  var selectedText;
> +  assert.waitFor(function() {
> +    selectedText = controller.tabs.activeTabWindow.getSelection();
> +    return selectedText.toString().toLowerCase() === SEARCH_TEXT;

Why toLowerCase? Have you made sure that the 'Match case' setting is disabled?

@@ +57,5 @@
> +  assert.waitFor(function() {
> +    selectedText = controller.tabs.activeTabWindow.getSelection();
> +    var secondSelection = selectedText.getRangeAt(0).startOffset;
> +    return secondSelection !== firstSelection;
> +  }, "Next occurence of the text has been selected");

I think we should add two methods to the library which let us search for the next and the previous selection. Otherwise this code will be repeated too often.
Attachment #8371351 - Flags: review?(hskupin) → review-
(In reply to Henrik Skupin (:whimboo) from comment #20)
@@ +45,5 @@
> @@ +45,5 @@
> > +  // Check that the correct word has been highlighted
> > +  var selectedText;
> > +  assert.waitFor(function() {
> > +    selectedText = controller.tabs.activeTabWindow.getSelection();
> > +    return selectedText.toString().toLowerCase() === SEARCH_TEXT;
> 
> Why toLowerCase? Have you made sure that the 'Match case' setting is disabled?

I think this comes from the homologue test for regular firefox on which this is based:
http://hg.mozilla.org/qa/mozmill-tests/file/fc717c0f116b/firefox/tests/functional/testFindInPage/testFindInPage.js#l80
If we want to get better in our tests we should not simply copy and paste, but think about what is better.
Looks like the preference for case sensitive exists but doesn't work on metro.
I logged bug 970875 for this.
Attached patch 6.patch (obsolete) — Splinter Review
Details about the patch:
1. Created 2 new methods in toolbars.js::FindBar ( findNext & findPrevious ).
3. Updated test to use them.
2. Updated FindBar.close() method to use a force flag.
Attachment #8371351 - Attachment is obsolete: true
Attachment #8374093 - Flags: review?(andrei.eftimie)
Attachment #8374093 - Flags: review?(andreea.matei)
Comment on attachment 8374093 [details] [diff] [review]
6.patch

Review of attachment 8374093 [details] [diff] [review]:
-----------------------------------------------------------------

::: metrofirefox/lib/ui/toolbars.js
@@ +190,5 @@
>     *
>     * @param {string} aMethod
>     *        Specifies a method for closing the find bar
>     */
> +  close: function FindBar_close(aMethod, force) {

nit: please preserve the style guide rule we have to named arguments: aForce

@@ +199,5 @@
>  
> +    if(this.isOpen) {
> +      var findBar = this.getElement({type: "findbar"});
> +      if(force) {
> +        findBar.getNode().dismiss();

Move this code at the beginning of the function and instead of wrapping the rest in an else clause just return early. You can also use a single if clause to make both checks.

::: metrofirefox/tests/functional/testFindInPage/testFindInPage.js
@@ +12,5 @@
> +const TEST_DATA = BASE_URL + "layout/mozilla_community.html";
> +
> +const SEARCH_TEXT = "community";
> +
> +const FIND_CASESENSITIVE_PROP = "accessibility.typeaheadfind.casesensitive";

For preferences use PREF_[...] for a constant name.

@@ +46,5 @@
> +    position: null
> +  };
> +  assert.waitFor(function () {
> +    firstSelection.text = controller.tabs.activeTabWindow.getSelection();
> +    return firstSelection.text.toString().toLowerCase() === SEARCH_TEXT;

Add a todo comment to remove the lowercase transform once bug 968739 lands.
Attachment #8374093 - Flags: review?(andrei.eftimie)
Attachment #8374093 - Flags: review?(andreea.matei)
Attachment #8374093 - Flags: review-
Attached patch 6.1.patch (obsolete) — Splinter Review
(In reply to Andrei Eftimie from comment #25)
> Comment on attachment 8374093 [details] [diff] [review]
> 6.patch
> 
> Review of attachment 8374093 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> @@ +46,5 @@
> > +    position: null
> > +  };
> > +  assert.waitFor(function () {
> > +    firstSelection.text = controller.tabs.activeTabWindow.getSelection();
> > +    return firstSelection.text.toString().toLowerCase() === SEARCH_TEXT;
> 
> Add a todo comment to remove the lowercase transform once bug 968739 lands.

I think the bug you are referring is bug 970875. Should we set the CASESENSITIVE preference to 1 in setup ? In this case we will need this TO DO once that is landed, otherwise the toLowerCase is needed here.
Attachment #8374093 - Attachment is obsolete: true
Attachment #8378993 - Flags: review?(andrei.eftimie)
Attachment #8378993 - Flags: review?(andreea.matei)
(In reply to daniel.gherasim from comment #26)
> I think the bug you are referring is bug 970875. Should we set the
> CASESENSITIVE preference to 1 in setup ? In this case we will need this TO
> DO once that is landed, otherwise the toLowerCase is needed here.
Yes, that bug. I probably miss-copied the id.

As I've said, leave toLowerCase for now, but before that we need to put a comment referencing the bug and add a short description. This comment should be inserted for every instance of toLowerCase where we would need to remove it later.

See the style guide for how to format this for a temporary workaround:
https://developer.mozilla.org/en-US/docs/Mozmill_Tests/Mozmill_Style_Guide#Commenting
Comment on attachment 8378993 [details] [diff] [review]
6.1.patch

Review of attachment 8378993 [details] [diff] [review]:
-----------------------------------------------------------------

This looks good, but it misses the TODO with bug references Andrei mentioned.
Attachment #8378993 - Flags: review?(andrei.eftimie)
Attachment #8378993 - Flags: review?(andreea.matei)
Attachment #8378993 - Flags: review-
Attached patch 6.2.patch (obsolete) — Splinter Review
Added the TO DO.

Hope now it's ok!
Attachment #8378993 - Attachment is obsolete: true
Attachment #8380500 - Flags: review?(andrei.eftimie)
Attachment #8380500 - Flags: review?(andreea.matei)
Comment on attachment 8380500 [details] [diff] [review]
6.2.patch

Review of attachment 8380500 [details] [diff] [review]:
-----------------------------------------------------------------

Yes, it's fine now. Asking review from Henrik as well.
Attachment #8380500 - Flags: review?(hskupin)
Attachment #8380500 - Flags: review?(andrei.eftimie)
Attachment #8380500 - Flags: review?(andreea.matei)
Attachment #8380500 - Flags: review+
The final review here should really have been gone to Dave during my absence. I will try to get to it soon.
Comment on attachment 8380500 [details] [diff] [review]
6.2.patch

Review of attachment 8380500 [details] [diff] [review]:
-----------------------------------------------------------------

::: metrofirefox/lib/ui/toolbars.js
@@ +190,5 @@
>     *
>     * @param {string} aMethod
>     *        Specifies a method for closing the find bar
>     */
> +  close: function FindBar_close(aMethod, aForce) {

I don't see a jsdoc entry for aForce here. Also aMethod is optional.

@@ +193,5 @@
>     */
> +  close: function FindBar_close(aMethod, aForce) {
> +    var findBar = this.getElement({type: "findbar"});
> +
> +    if(!this.isOpen || aForce) {

Why do you want to dismiss the find bar when it is not visible?

@@ +203,4 @@
>      function onTransitionEnd() {
>        transitioned = true;
>      }
> +    var method = aMethod || "button";

Checks for function parameters should come first in a function. So please move this up again.

@@ +241,5 @@
> +    var selectedText = this._controller.tabs.activeTabWindow.getSelection();
> +    return {
> +      text: selectedText.toString(),
> +      position: selectedText.getRangeAt(0).startOffset
> +    }

I really would return what you get from getSelection() and not construct a custom dict.

@@ +256,5 @@
> +    var selectedText = this._controller.tabs.activeTabWindow.getSelection();
> +    return {
> +      text: selectedText.toString(),
> +      position: selectedText.getRangeAt(0).startOffset
> +    }

Same here.

::: metrofirefox/tests/functional/testFindInPage/manifest.ini
@@ +1,1 @@
> +[testFindInPage.js]

Can we call the sub folder name 'testFindBar' instead? Otherwise we have duplication which is not necessary.

::: metrofirefox/tests/functional/testFindInPage/testFindInPage.js
@@ +28,5 @@
> +  toolBar.findBar.close("", true);
> +}
> +
> +/**
> + * Test find in page functionality

Mind adding the bug number here?

@@ +38,5 @@
> +  toolBar.findBar.open();
> +
> +  // Do a search
> +  var findBarTextbox = toolBar.findBar.getElement({type: "textbox"});
> +  findBarTextbox.sendKeys(SEARCH_TEXT);

Don't we have findBar.search() yet? We should move that code over there.

@@ +44,5 @@
> +  // Check that the correct word has been highlighted
> +  var firstSelection = {
> +    text: "",
> +    position: null
> +  };

Why it has to be initialized that way? 'var firstSelection' should be totally enough.

@@ +46,5 @@
> +    text: "",
> +    position: null
> +  };
> +  assert.waitFor(function () {
> +    firstSelection.text = controller.tabs.activeTabWindow.getSelection();

I don't see a reason why we have to store that under '.text'. Why is that necessary?

As best I would like to see findBar.getSelection(), which would make it more obvious what we are doing here.

@@ +51,5 @@
> +
> +    // Bug 970875
> +    // The "accessibility.typeaheadfind.casesensitive" pref doesn't work
> +    // Remove toLowerCase once the preference is fixed
> +    return firstSelection.text.toString().toLowerCase() === SEARCH_TEXT;

I still don't think that we should call toLowerCase() here, and really search for the text with the wanted capitalization. Otherwise we do not find regressions for the default behavior of Firefox.

@@ +55,5 @@
> +    return firstSelection.text.toString().toLowerCase() === SEARCH_TEXT;
> +  }, "Text has been selected");
> +
> +  // Remember position of the first selection
> +  firstSelection.position = firstSelection.text.getRangeAt(0).startOffset;

Same as above. Unnecessary copy of the actual data.
Attachment #8380500 - Flags: review?(hskupin) → review-
(In reply to Henrik Skupin (:whimboo) from comment #32)

> @@ +51,5 @@
> > +
> > +    // Bug 970875
> > +    // The "accessibility.typeaheadfind.casesensitive" pref doesn't work
> > +    // Remove toLowerCase once the preference is fixed
> > +    return firstSelection.text.toString().toLowerCase() === SEARCH_TEXT;
> 
> I still don't think that we should call toLowerCase() here, and really
> search for the text with the wanted capitalization. Otherwise we do not find
> regressions for the default behavior of Firefox.
> 

The default behavior of firefox is to ignore capitalization so the _CASESENSITIVE_ preference is default false. So I think toLowerCase() is needed here unless we compare the selection with the expected word on the page, or set the preference to true initially, (but we found out it's not working properly in metro).
Oh, my bad. Looks like I was wrong here. Given that we should use the default behavior of Firefox, we should not turn on case sensitivity, but keep the toLowerCase() call. Not sure why I have suggested something else earlier, but at least we found a bug in Metro!
Attached patch 6.3.patch (obsolete) — Splinter Review
(In reply to Henrik Skupin (:whimboo) from comment #32)
> > +    if(!this.isOpen || aForce) {
> 
> Why do you want to dismiss the find bar when it is not visible?
> 

Right. We should call dismiss and return always if the force flag is active, given that we'll use that in tearDownModules. Also, dismiss() is already checking if the findbar is visible and doesn't throw anything if it's not.

> Don't we have findBar.search() yet? We should move that code over there.

We do have findBar.type() => used that.
Attachment #8380500 - Attachment is obsolete: true
Attachment #8387525 - Flags: review?(andrei.eftimie)
Attachment #8387525 - Flags: review?(andreea.matei)
Comment on attachment 8387525 [details] [diff] [review]
6.3.patch

Review of attachment 8387525 [details] [diff] [review]:
-----------------------------------------------------------------

::: metrofirefox/lib/ui/toolbars.js
@@ +277,5 @@
>      var method = aMethod || "menu";
>      var transitioned = false;
>  
> +    if(this.isOpen) {
> +      assert.fail("Find bar already opened");

Do we need this?
We will fail further down waiting for the `transition` to be updated.

::: metrofirefox/tests/functional/testFindBar/testFindInPage.js
@@ +47,5 @@
> +  var firstSelection = selectedText.getRangeAt(0).startOffset;
> +
> +  // Check that the next occurence has been highlighted
> +  assert.waitFor(function () {
> +    selectedText = toolBar.findBar.findNext();

This should not be called inside the waitFor loop. We want to click on next only once.

@@ +57,5 @@
> +               "The correct text has been selected");
> +
> +  // Check that the first selection has been reselected
> +  assert.waitFor(function () {
> +    selectedText = toolBar.findBar.findPrevious();

Same here.
Attachment #8387525 - Flags: review?(andrei.eftimie)
Attachment #8387525 - Flags: review?(andreea.matei)
Attachment #8387525 - Flags: review-
Attached patch 6.4.patch (obsolete) — Splinter Review
Updated patch as requested.
Attachment #8387525 - Attachment is obsolete: true
Attachment #8389130 - Flags: review?(andrei.eftimie)
Attachment #8389130 - Flags: review?(andreea.matei)
Comment on attachment 8389130 [details] [diff] [review]
6.4.patch

Review of attachment 8389130 [details] [diff] [review]:
-----------------------------------------------------------------

Will need a small update since it's no longer apply cleanly.
With the mentioned changes this has r+ from me.

::: metrofirefox/lib/ui/toolbars.js
@@ +187,5 @@
>  
>    /**
>     * Close the find bar
>     *
> +   * @param {Object} aSpec

The whole aSpec object is optional

@@ +194,2 @@
>     *        Specifies a method for closing the find bar
> +   * @param {boolean} [aSpec.force]

Lets also mark the default value here.
Attachment #8389130 - Flags: review?(andrei.eftimie)
Attachment #8389130 - Flags: review?(andreea.matei)
Attachment #8389130 - Flags: review+
Attached patch 7.patch (obsolete) — Splinter Review
Updated patch and asked Henrik for a final check.
Attachment #8389130 - Attachment is obsolete: true
Attachment #8390516 - Flags: review?(hskupin)
Comment on attachment 8390516 [details] [diff] [review]
7.patch

Review of attachment 8390516 [details] [diff] [review]:
-----------------------------------------------------------------

Looks fine. Just remove the comment and we can get this landed.

::: metrofirefox/tests/functional/testFindBar/testFindInPage.js
@@ +20,5 @@
> +  aModule.toolBar = new toolbars.ToolBar(aModule.controller);
> +}
> +
> +function teardownModule(aModule) {
> +  // Force closing findBar

nit: comment is not necessary.
Attachment #8390516 - Flags: review?(hskupin) → review+
Attached patch 8.patchSplinter Review
Final patch.
Attachment #8390516 - Attachment is obsolete: true
Attachment #8391052 - Flags: review?(andrei.eftimie)
Comment on attachment 8391052 [details] [diff] [review]
8.patch

Review of attachment 8391052 [details] [diff] [review]:
-----------------------------------------------------------------

Landed:
http://hg.mozilla.org/qa/mozmill-tests/rev/421d1073361e (default)
Attachment #8391052 - Flags: review?(andrei.eftimie)
Attachment #8391052 - Flags: review+
Attachment #8391052 - Flags: checkin+
Status: ASSIGNED → RESOLVED
Closed: 6 years ago6 years ago
Resolution: --- → FIXED
OS: Windows 8 Metro → Windows 8.1
Product: Mozilla QA → Mozilla QA Graveyard
You need to log in before you can comment on or make changes to this bug.