"Search with css selectors" is confused by page mutations

RESOLVED FIXED in Firefox 43

Status

defect
RESOLVED FIXED
4 years ago
Last year

People

(Reporter: arni2033, Assigned: pbro, Mentored)

Tracking

Trunk
Firefox 43

Firefox Tracking Flags

(firefox43 fixed)

Details

(Whiteboard: [good next bug][lang=js])

Attachments

(3 attachments, 6 obsolete attachments)

STR:
1. Open attachment page with 80 "<br>"s and one "<a>" or this page:
data:text/html,<body onload="var str='';for (i=0;i<80;i++) str+='<br>'; document.body.innerHTML= str + '<a>' ">
2. Open Inspector, make sure that html is long enough - at least three times higher than Inspector's height
3. Search for CSS selector "a" (without qoutes)
4. Delete that <a> node via context menu or JavaScript
5. Focus the "Search for CSS selector" findbar, press Enter several times

Result:
The html nodes in inspector move as if I were scrolling up

Expectation:
Nothing should happen (when you delete the last element that matches the CSS selector, and then try to search with that selector Again)
Posted video screencast.webm
This fails here:
https://dxr.mozilla.org/mozilla-central/source/browser/devtools/inspector/inspector-panel.js#464

With:
Protocol error (unknownError): Error: findCssSelector received element not inside document

Which means the actor method 'getUniqueSelector' fails, and because it's an unexpected failure (the inspector panel is not being destroyed at the moment), we log it.

We shouldn't even be executing 'onNewSelection' in this case anyway. As to why the markup-view panel scrolls back up a little bit each time, I don't know, I haven't investigated that bit. But I suspect it would go away if we fix the root cause by preventing to trigger a new node selection when search returns a detached node.
Status: UNCONFIRMED → NEW
Ever confirmed: true
Not too sure about how involved this bug is, so I'm marking it as good *next* bug, but it could be simple. I can mentor people who want to work on this, but I'll be away for 3 weeks starting now. So if someone wants to work on this in the meantime:

- be sure to check https://wiki.mozilla.org/DevTools/GetInvolved
- come and say hi on our IRC channel (#devtools) and ask for help there if you need
- familiarize yourself with working with HG and making patches: https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/How_to_Submit_a_Patch
- the interesting code for this bug will be:
  - on the server-side, i.e. on the page, (where the actual search is done): https://dxr.mozilla.org/mozilla-central/source/toolkit/devtools/server/actors/inspector.js
  - on the client-side, i.e. the devtools UI (where the search results are shown and where DOM nodes are shown in a tree): https://dxr.mozilla.org/mozilla-central/source/browser/devtools/inspector/inspector-panel.js and https://dxr.mozilla.org/mozilla-central/source/browser/devtools/markupview/markup-view.js
Mentor: pbrosset
Whiteboard: [good next bug][lang=js]
I found anoter STR (adding first search result). I guess it refers to this bug (But maybe not!!!):

1. Open page   data:text/html,<body><script>setTimeout(function(){document.body.innerHTML="<div>";document.title="done"},8*1000);</script>
2. Open devtools, focus "Search with CSS selectors" field, type "div" (without quotes)
[You would see that there's no such element yet. It's OK]
3. Wait until <div> element appears (8 seconds in my example)
4. Press Enter several times

Result:       No visible changes (panel doesn't scroll back up - I checked it with lots of nodes)
Expectations: <div> node in markup-view should be highlighted, because now it exists
Hi! I'm interested in to fix this bug. May you assign me ?
(In reply to Ali Movahedi from comment #5)
> Hi! I'm interested in to fix this bug. May you assign me ?
We usually wait until a first patch is attached before assigning the bug.
Have you taken a look at the code? Do you already have an idea on where to start?
Let me know your questions and I'll provide technical details if needed.
Posted patch Fix scrolling up (obsolete) — Splinter Review
It's my first next bug. I'm newbie in next bug.

I've searched files, I think that problem is https://dxr.mozilla.org/mozilla-central/source/browser/devtools/inspector/selector-search.js#295. because if conditions will be true when we press enter, then inspector will increase searchIndex. but inspector should search again.

anyway I fixed this bug.
Attachment #8654509 - Flags: review?(pbrosset)
Thanks for this patch, I'll start reviewing it now.
Assignee: nobody → ali_movahedi
Status: NEW → ASSIGNED
Posted patch Fix scrolling up #2 (obsolete) — Splinter Review
I've made another patch. I think it's better than previous patch.
Attachment #8655352 - Flags: review?(pbrosset)
Attachment #8654509 - Attachment is obsolete: true
Attachment #8654509 - Flags: review?(pbrosset)
Comment on attachment 8655352 [details] [diff] [review]
Fix scrolling up #2

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

::: browser/devtools/inspector/selector-search.js
@@ +292,5 @@
>      let query = this.searchBox.value;
>      switch(aEvent.keyCode) {
>        case aEvent.DOM_VK_RETURN:
>          if (query == this._lastSearched && this._searchResults) {
> +          this._queryNodes(query).then(list => {

Your fix does solve the problem as stated in comment 0, so that's nice. However, I think there might be a better way to solve this more efficiently.
Indeed, the |if| statement you put your code in was made for when we already have done a search and we just want to select an existing result. So this is a case where we're not needing to do another request to the server (as in, the devtools debugger server, where matching nodes are retrieved).
By adding a call to |_queryNodes| in here, you made it call the server every time.

Here's another approach: the problem here is that if we search again for the same query after some markup mutations occurred on the page, we may either be finding nodes that have been deleted, or we may not be finding new nodes that have been added.
To solve this, we could simply reset the last query results whenever there's a markup mutation.

You can do this by using the |markupmutation| event from the InspectorPanel.
In |SelectorSearch| this means something like this:
|this.inspector.on("markupmutation", this._onMarkupMutation);|

And in this new callback, you could do something like this:

  _onMarkupMutation: function() {
    // Reset previous search results on markup-mutations to make sure we search
    // again after nodes have been added/removed/changed.
    this._searchResults = null;
    this._lastSearched = null;
  },

Does that make sense?
Attachment #8655352 - Flags: review?(pbrosset)
Posted patch Fix scrolling up #3 (obsolete) — Splinter Review
I didn't know about |markupmutation| event. Thanks.
Attachment #8655385 - Flags: review?(pbrosset)
Comment on attachment 8655385 [details] [diff] [review]
Fix scrolling up #3

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

This looks great.
Have you already written an automated mochitest test? We try and create tests for all bugs we fix and all features we add, so that we can prevent regressions when making future changes.
We have some docs here: https://wiki.mozilla.org/DevTools/Hacking#Running_the_Developer_Tools_Tests
I think the best type of test for this is a browser mochitest. I suggest adding a new file inside browser/devtools/inspector/test
You'll see there are already at least 5 search-related tests in this directory which you can use for inspiration. Please add a new one there (don't forget to reference it in browser/devtools/inspector/test/browser.ini too). This test should check that the use cases described in comment 0 and comment 4 work correctly.
Let me know if you have questions about this.
Attachment #8655385 - Flags: review?(pbrosset) → feedback+
Posted patch Patch + Test (obsolete) — Splinter Review
my test is not complete. I got an error. Please take a look my patch if you may.
Flags: needinfo?(pbrosset)
Comment on attachment 8656560 [details] [diff] [review]
Patch + Test

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

This test looks pretty good! Great start. It doesn't need much to actually pass. I've made a bunch of comments below in the code.
Could you please mark older attachments as obsolete when you upload new ones? That makes it simpler to know which patch should be reviewed/tested/checked-in. Thanks.

::: browser/devtools/inspector/test/browser_inspector_search-06.js
@@ +2,5 @@
> + * License, v. 2.0. If a copy of the MPL was not distributed with this
> + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
> +"use strict";
> +
> +// Check previous search will be destroyed after markup-mutations

nit: I think this comment could be more self-explanatory:
// Check that searching again for a node after that node was removed does work correctly.
Or something like this.

@@ +10,5 @@
> +add_task(function* () {
> +  let { inspector, testActor } = yield openInspectorForURL(TEST_URL);
> +  let { searchBox } = inspector;
> +
> +  yield focusSearchBoxUsingShortcut(inspector.panelWin);

It's often a good idea to add info("...") statements in tests because they help when reading logs later when tests fail.

So for instance, right before this line, you could add something like:

info("Searching for test node #d1");

@@ +13,5 @@
> +
> +  yield focusSearchBoxUsingShortcut(inspector.panelWin);
> +
> +  for (let key of ["#", "d", "1"]) {
> +

nit: no need for the empty line.

@@ +20,5 @@
> +    yield eventHandled;
> +
> +    info("Got command event. Waiting for search query to complete");
> +    yield inspector.searchSuggestions._lastQuery;
> +

nit: no need for the empty line here.

@@ +24,5 @@
> +
> +  }
> +
> +  is(!searchBox.classList.contains("devtools-no-search-result"), true,
> +     "There is no element with specified CSS selector");

Here you're using is(a, b) with b being a boolean. This means you should probably be using ok(a) instead:

ok(!searchBox.classList.contains("devtools-no-search-result"),
   "A result was found");

@@ +26,5 @@
> +
> +  is(!searchBox.classList.contains("devtools-no-search-result"), true,
> +     "There is no element with specified CSS selector");
> +
> +  yield testActor.eval(`document.body.removeChild(document.getElementById("d1"))`);

Shorter this way:
yield testActor.eval("document.getElementById(\"d1\").remove()");

The other thing is that here, you're not waiting for the node to be shown as removed in the markup-view.
You want the test to make sure the node appears as removed in the markup-view before doing other things.
You could do this with:

let onUpdated = inspector.once("inspector-updated");
yield testActor.eval("document.getElementById(\"d1\").remove()");
yield onUpdated;

@@ +33,5 @@
> +  EventUtils.synthesizeKey("VK_RETURN", {}, inspector.panelWin);
> +  yield eventHandled;
> +
> +  info("Got command event. Waiting for search query to complete");
> +  yield inspector.searchSuggestions._lastQuery;

The last 5 lines also appear earlier in the test (in the for loop), so they're a good candidate to move into a separate function:

function* synthesizeSearchKey(key, inspector) {
  let eventHandled = once(inspector.searchBox, "command", true);
  EventUtils.synthesizeKey(key, {}, inspector.panelWin);
  yield eventHandled;
  yield inspector.searchSuggestions._lastQuery;
}

You can put this somewhere at the end of the test, and then just call it both here and inside the for loop, therefore making the code shorter and easier to maintain.

@@ +36,5 @@
> +  info("Got command event. Waiting for search query to complete");
> +  yield inspector.searchSuggestions._lastQuery;
> +
> +  is(!searchBox.classList.contains("devtools-no-search-result"), false,
> +     "There is no element with specified CSS selector");

Same remark here about using ok instead of is:

ok(searchBox.classList.contains("devtools-no-search-result"),
   "No results were found");
Attachment #8656560 - Flags: feedback+
Flags: needinfo?(pbrosset)
Posted patch path + test (obsolete) — Splinter Review
Thanks.

I've completed my test.
Attachment #8655352 - Attachment is obsolete: true
Attachment #8655385 - Attachment is obsolete: true
Attachment #8656560 - Attachment is obsolete: true
Attachment #8658094 - Flags: review?(pbrosset)
Comment on attachment 8658094 [details] [diff] [review]
path + test

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

Thanks for the new test, it seems to work fine.
I would just like to make a point about making the test code more easily readable.
And instead of going on and on with long comments, I'm just going to make a few ones below, and then attach an updated patch to show you what I have in mind.

::: browser/devtools/inspector/test/browser_inspector_search-06.js
@@ +2,5 @@
> + * License, v. 2.0. If a copy of the MPL was not distributed with this
> + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
> +"use strict";
> +
> +// Check that searching again for a node after that node was removed does work correctly.

nit: since the test now also uses the search after a node was added, this comment should be changed to reflect this.

@@ +16,5 @@
> +    let eventHandled = once(inspector.searchBox, "command", true);
> +    EventUtils.synthesizeKey(key, {}, inspector.panelWin);
> +    yield eventHandled;
> +    info("Got command event. Waiting for search query to complete");
> +    yield inspector.searchSuggestions._lastQuery;

The last 5 lines are repeated several times in the test. This is something I mentioned in my previous review I believe. We should always try to minimize the amount of duplicated code since that makes it harder to maintain.
It shouldn't be too hard to extract this into a separate function that you can call over and over again.
This would also make the test far easier to read (right now it's a bit long whereas the actual test logic is really short, removing these lines would help make the test logic stand out easily).

@@ +20,5 @@
> +    yield inspector.searchSuggestions._lastQuery;
> +  }
> +
> +  ok(!inspector.searchBox.classList.contains("devtools-no-search-result"),
> +  "A result was found");

formatting nitpicking: please vertically align the string with the first paramater of 'ok':

ok(!inspector.searchBox.classList.contains("devtools-no-search-result"),
   "A result was found");

In fact, to further clarify my previous point, I believe you could wrap this into another function in order to make the test logic even easier to read:

assertHasResult(inspector);

function assertHasResult(inspector) {
  ok(!inspector.searchBox.classList.contains("devtools-no-search-result"),
     "A result was found");
}
Attachment #8658094 - Flags: review?(pbrosset)
Please do take a look at the changes I made in the test and let me know your thoughts. I believe it's a lot easier to read and maintain.
I've also pushed this to TRY to make sure the test passes on all platforms and to make sure no other tests fail too:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=38e3b626cd04
Attachment #8658094 - Attachment is obsolete: true
Comment on attachment 8658745 [details] [diff] [review]
Bug_1184525_-_Reset_previous_search_results_on_mar.diff

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

Ok try is green. I'm going to R+ this patch.
Although I uploaded this one, this is Ali's patch (it has the right author name), and I only refactored the test a little bit.
This is good for check-in.

Thanks Ali for your work. Don't hesitate to go take a look at http://firefox-dev.tools to find other similar bugs to work on!
Attachment #8658745 - Flags: review+
sorry had to back this out for test failures like https://treeherder.mozilla.org/logviewer.html#?job_id=4627733&repo=fx-team
Flags: needinfo?(ali_movahedi)
In case the logs expire, here's the relevant part (failed on linux debug):

 23:38:43     INFO -  260 INFO Entering test
23:38:43     INFO -  261 INFO Adding a new tab with URL: 'http://example.com/browser/browser/devtools/inspector/test/doc_inspector_search.html'
23:38:43     INFO -  262 INFO Waiting for event: 'load' on [object XULElement].
23:38:43     INFO -  263 INFO Got event: 'load' on [object XULElement].
23:38:43     INFO -  264 INFO URL 'http://example.com/browser/browser/devtools/inspector/test/doc_inspector_search.html' loading complete
23:38:43     INFO -  265 INFO Opening the inspector
23:38:43     INFO -  266 INFO Opening the toolbox
23:38:43     INFO -  267 INFO Console message: [JavaScript Warning: "Error in parsing value for 'text-anchor'.  Declaration dropped." {file: "resource://gre/modules/devtools/server/actors/highlighter.css" line: 269 column: 15 source: "  text-anchor: left;"}]
23:38:43     INFO -  268 INFO Making sure that the toolbox's frame is focused
23:38:43     INFO -  269 INFO Waiting for the inspector to update
23:38:43     INFO -  270 INFO Searching for test node #d1
23:38:43     INFO -  271 INFO Focusing search box
23:38:43     INFO -  272 INFO Waiting for event: 'focus' on [object XULElement].
23:38:43     INFO -  273 INFO TEST-PASS | browser/devtools/inspector/test/browser_inspector_search-06.js | Successfully retrieved the <key> node
23:38:43     INFO -  274 INFO TEST-PASS | browser/devtools/inspector/test/browser_inspector_search-06.js | Successfully retrieved keycode/key
23:38:43     INFO -  275 INFO Got event: 'focus' on [object XULElement].
23:38:43     INFO -  276 INFO Synthesizing key # in the search box
23:38:43     INFO -  277 INFO Waiting for event: 'command' on [object XULElement].
23:38:43     INFO -  278 INFO Console message: [JavaScript Warning: "Key event not available on GTK2: key="u" modifiers="shift, accel"" {file: "chrome://mochikit/content/tests/SimpleTest/EventUtils.js" line: 677}]
23:38:43     INFO -  279 INFO Got event: 'command' on [object XULElement].
23:38:43     INFO -  280 INFO Waiting for the search query to complete
23:38:43     INFO -  281 INFO Synthesizing key d in the search box
23:38:43     INFO -  282 INFO Waiting for event: 'command' on [object XULElement].
23:38:43     INFO -  283 INFO Got event: 'command' on [object XULElement].
23:38:43     INFO -  284 INFO Waiting for the search query to complete
23:38:43     INFO -  285 INFO Synthesizing key 1 in the search box
23:38:43     INFO -  286 INFO Waiting for event: 'command' on [object XULElement].
23:38:43     INFO -  287 INFO Got event: 'command' on [object XULElement].
23:38:43     INFO -  288 INFO Waiting for the search query to complete
23:38:43     INFO -  289 INFO TEST-PASS | browser/devtools/inspector/test/browser_inspector_search-06.js | There are search results
23:38:43     INFO -  290 INFO Removing node #d1
23:38:43     INFO -  291 INFO Pressing return button to search again for node #d1.
23:38:43     INFO -  292 INFO Synthesizing key VK_RETURN in the search box
23:38:43     INFO -  293 INFO Waiting for event: 'command' on [object XULElement].
23:38:43     INFO -  294 INFO TEST-UNEXPECTED-FAIL | browser/devtools/inspector/test/browser_inspector_search-06.js | Test timed out - expected PASS

As discussed with Ali, I'll take a look at this and try to reland the patch.
Flags: needinfo?(ali_movahedi)
Made a slight change to the test to use keypress instead of command.
Here's a try build with many re-triggers on linux debug that seems to show the problem is fixed: https://treeherder.mozilla.org/#/jobs?repo=try&revision=3ff851378835

r=test-only
Assignee: ali_movahedi → pbrosset
Attachment #8658745 - Attachment is obsolete: true
Attachment #8662793 - Flags: review+
https://hg.mozilla.org/mozilla-central/rev/1d7734f39705
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 43
See Also: → 1228675
Has STR: --- → yes
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.