Inspector searchbox should also provide a way to search plain text on the page.

RESOLVED FIXED in Firefox 45

Status

defect
P1
normal
RESOLVED FIXED
6 years ago
2 months ago

People

(Reporter: Optimizer, Assigned: bgrins)

Tracking

(Depends on 10 bugs, Blocks 1 bug, Regressed 1 bug, {dev-doc-complete})

unspecified
Firefox 45
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(relnote-firefox 45+)

Details

(Whiteboard: [polish-backlog][difficulty=hard][devtools-ux])

Attachments

(4 attachments, 8 obsolete attachments)

3.26 KB, patch
bgrins
: review+
bgrins
: checkin+
Details | Diff | Splinter Review
1.69 KB, patch
flod
: review+
bgrins
: checkin+
Details | Diff | Splinter Review
40 bytes, text/x-review-board-request
pbro
: review+
Details
1.45 KB, text/html
Details
Reporter

Description

6 years ago
Highlighting the node that contains that text as the result.

Since default behavior of the searchbox is to consider the input as query, we can add a character that is not part of CSS selectors, like '!'

'!some text' will select the node with the textContent matching "some text"
Simpler syntax: if you type "foo" (with single or double quotes), then nodes are search by their textContent.
I'm not sure it's a good idea to support that.

First, I think it's easier if the searchbox is for selectors only. By easier I mean "easy to understand" from the user point of view. I'm not sure we want to make the searchbox more complex just to support this feature.

And you can always search for text in the page content and then select the node.

But I can be convinced otherwise :)
Reporter

Comment 3

6 years ago
(In reply to Paul Rouget [:paul] from comment #2)
> I'm not sure it's a good idea to support that.
> 
> First, I think it's easier if the searchbox is for selectors only. By easier
> I mean "easy to understand" from the user point of view. I'm not sure we
> want to make the searchbox more complex just to support this feature.

True it will become complex. But then there we have the Debugger's Search Box :P

> And you can always search for text in the page content and then select the
> node.
> 
> But I can be convinced otherwise :)

What if the node is hidden or something like that, due to which the text search will not show the icon. We are talking about Developers here, who will be the main users of this search box. They tend to do stuff like that :)
Reporter

Comment 4

6 years ago
s/show the icon/show the node or selection
(In reply to Girish Sharma [:Optimizer] from comment #3)
> What if the node is hidden or something like that, due to which the text
> search will not show the icon. We are talking about Developers here, who
> will be the main users of this search box. They tend to do stuff like that :)

That's a very specific case. Confusing 99% of the usages for a 1% use case.
(In reply to Paul Rouget [:paul] from comment #5)
> (In reply to Girish Sharma [:Optimizer] from comment #3)
> > What if the node is hidden or something like that, due to which the text
> > search will not show the icon. We are talking about Developers here, who
> > will be the main users of this search box. They tend to do stuff like that :)
> 
> That's a very specific case. Confusing 99% of the usages for a 1% use case.

I agree with Paul [0]. In a debugger, it's a very common think to search for strings something across files. In an inspector, it's very common to search for selectors. DOM should be treated differently than JS.

[0] http://en.wikipedia.org/wiki/Pareto_principle
(In reply to Victor Porof [:vp] from comment #6)
> I agree with Paul [0]. In a debugger, it's a very common think to search for
> strings something across files. In an inspector, it's very common to search
> for selectors. DOM should be treated differently than JS.

If this searchbox were visually confined to the style inspector, I would agree. As it is now, however, it almost looks like it targets the markup panel (it's right above it!), in which case searching for anything inside the markup would be the expected thing for the uninitiated. I would argue that searching for CSS selectors, although extremely useful, should be hidden behind an operator, like we do in the debugger. 

Either that, or move the selector filter inside the sidebar, like we do in the variables view.
Reporter

Comment 8

6 years ago
So... What should we do here, we might want to change the placeholder accordingly.

Updated

6 years ago
Blocks: :PaulFx21
Reporter

Comment 9

6 years ago
(In reply to Panos Astithas [:past] from comment #7)
> Either that, or move the selector filter inside the sidebar, like we do in
> the variables view.

That cannot be done as the sidebar does not contain the list of all the selectors (as compared to variables view, in which the content from which the search is made is already present in sidebar). In this case, markupview is the right place for the searchbox as the search happens from the content of the page dom, and markupview is the only thing displaying the full page dom.
Assignee

Comment 10

5 years ago
Just describing the ideal usage for this search feature from my point of view.  It would filter not just by textContent or selector, but by any content that is visible in the markup view.  So if you had:

<body class="container">
<link rel="stylesheet" href="whatever.css" />
Hello
</body>

Searching "rel" could highlight the attribute name, searching "Hello" would highlight the text content, searching for "body" or "link" would highlight the tag name.  And you enter a normal selector as currently, like .container, that could still autocomplete and highlight.  If there are multiple matches, you can ctrl+g your way through them and it would jump you to each match in the markup view.

There would be cases where you may want to ignore certain text, like if you search for "body" here, you probably don't need to also highlight the </body> as a search result.

If any of these search results happen on elements that are not currently loaded into the markup view, they should be loaded into the tree to show as results.
Just to add in my personal experience. When I first started using the inspector search feature and wanted to find a particular DOM element with a class/id, I simply enter the class/id as is. The fact that the search returned nothing was a bit daunting. Only a day later did I figure I should try using selectors in the search bar, but at that point I was still trying to figure out how the inspector search worked. It was also important to me that my search term was highlighted similar to the use case described by Brian.

I was originally thinking the solution to this problem was to utilize CodeMirror's search capability to handle plain text search, and somehow integrate that with selector search.
FWIW Firebug's HTML panel search works like described in comment #10 since issue 6748[1] got implemented.
Can be tried out in the current alphas.

Sebastian

[1] http://code.google.com/p/fbug/issues/detail?id=6748
I was thinking about the markup-view search feature just yesterday and had no idea this bug existed.
I completely agree with comment 11 in that, even though css selectors are a good way to search for nodes, they are "hard" to type. If you have a class or id in mind, you're tempted to type its name in the search box and forget about the . or # characters.

So my opinion on this is, on top of searching for selectors as today, we should also:
- allow searching for attributes without the . or # or []
- allow searching on text content
- remember recently inspected nodes per page
With this, the search box could not only become a way to search but also a way to quickly jump to often inspected nodes on the page.

We could either do like the debugger and depending on which first char you type, activate a different search mode, or always search on all, and sort the results in the drop-down (I think this would be a lot more useful).
Reporter

Comment 14

5 years ago
(In reply to Patrick Brosset [:pbrosset] [:patrick] from comment #13)
> [...]
> - remember recently inspected nodes per page

What do you mean by this ?

> With this, the search box could not only become a way to search but also a
> way to quickly jump to often inspected nodes on the page.
> 
> We could either do like the debugger and depending on which first char you
> type, activate a different search mode, or always search on all, and sort
> the results in the drop-down (I think this would be a lot more useful).

The issue here is that text without any preceding operator could also mean that its a css tag selector.

FWIW, Chrome searches on both text and selector side by side too.
Assignee

Updated

5 years ago
Duplicate of this bug: 881986
Duplicate of this bug: 986696

Comment 17

5 years ago
Related bug:  #840981 - "The text placeholder in the searchbox should be about CSS selector, not about HTML search". This is about making it clear in the UI that the search is not a simple text search.
It seems to me that this problem is partly caused by users not understanding that they can only search by CSS selectors.

Comment 18

5 years ago
Working link: bug 840981 .
Agreed. I also mentioned this in bug 986696. Affordances[1] are key.

Still, I think these are two separate bugs: one for making it clear to users that they can search by CSS selectors, and one for also accepting plain text. The latter is still important, in my opinion, because some things (like attributes and written content) are much easier to search for using plain text.

[1] http://pages.citebite.com/a3u0n6s2n4mux
Assignee

Updated

5 years ago
Depends on: 991745
I think if you search "main" it should show nodes containing that class or id or attribute. Like:

(ma)
 * .main
 * #main-element

I've definitely done that before where I've typed the class name without the dot and got momentarily confused as to why it wasn't showing up. That alone would cover a lot of use cases I think, and would be a good stepping stone to some kind of full text search.
(In reply to Heather Arthur [:harth] from comment #20)
> [...] That alone
> would cover a lot of use cases I think, and would be a good stepping stone
> to some kind of full text search.
Yes! Totally agree with this first step approach. Simple to do, and immediate benefits.
Posted patch better-selector-search.patch (obsolete) — Splinter Review
Here's a patch for what Heather mentioned in comment 20.
It makes a test fail and needs new tests to be written, but it works.

Do you mind applying it locally and playing with it, see if that could be helpful to users?

The basic behavior is:
- type a "real" selector, like "#myId" or ".my-class" or "div .class", ... and everything will work as before
- type just a word, like "div", and as before the selector search will search for the corresponding tags, but will also search for matching classes and IDs: ".div" and "#div". So the autocomplete list will display:
  * div
  * .div
  * #div
if there happens to be a div on the page, an element with class .div and an element with id #div.

Feedback on this would be great.
I still think that full text search is a thing we should do (to find other attributes or textContent for instance), and maybe we should even do fuzzy text search (we have a fuzzy matcher here: toolkit/devtools/gcli/source/lib/gcli/util/spell.js), but this first step is interesting.
Attachment #8418651 - Flags: feedback?(fayearthur)

Comment 23

5 years ago
Before reading this bug, it never occurred to me it would search selectors.  Generally when I want to search for selectors I pin the console and use $() if the current page has jQuery, or document.querySelectorAll() if it doesn't.  Especially if I am looking for more than one element, it is easier (for me) since they are right next to each other in the variables view, and I can further refine what I see there.  With the crosshairs that shows up next to each of the elements in the console/variables view, it seems easier to use the console pinned under the inspector to write more complex queries and the search to do full text.  Just my 2 cents.
Improving the interface to clearly indicate that selectors are needed would be a big improvement. It may make sense to do this even if search is expanded to support other types of content.

There have been many comments about this confusion.

> Just to add in my personal experience. When I first started using the
> inspector search feature and wanted to find a particular DOM element with a
> class/id, I simply enter the class/id as is. The fact that the search
> returned nothing was a bit daunting. Only a day later did I figure I should
> try using selectors in the search bar...

> I was using Firefox Developer Tools for a few months before I realized that
> the search box only accepts CSS selectors...

> I've definitely done that before where I've typed the class name without the
> dot and got momentarily confused as to why it wasn't showing up...

> Before reading this bug, it never occurred to me it would search selectors...
Comment on attachment 8418651 [details] [diff] [review]
better-selector-search.patch

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

I love this. The only thing is I would make the ordering of the list more consistent, like node names first, then ids, then class names. Or whatever you think might be best.
Attachment #8418651 - Flags: feedback?(fayearthur) → feedback+
Thanks for the feedback, I've started changing the sorting to be able to group results by ID, class and tags. 
I've also fixed 2 inspector failing tests that needed to be updated.

This is still a WIP because it makes the styleeditor tag completion fail. And to make it work, I'd have to copy/paste code from the inspector to the styleeditor, which would be a shame. In fact, when it comes to selector suggestions, we have a little bit of code in the client, to get the completed string and compute the "state" of this selector string, then a bit of code on the server to get suggestions, and finally, again some code on the client to prepare results to be shown in the completion popup.
It seems to me that most of the client-side work should be centralized server-side, therefore avoiding code duplication (especially because the duplicated code isn't straightforward at all).

Also, refactoring things a little bit on the server would serve as a foundation for a future fuzzy-matching text search.
Attachment #8418651 - Attachment is obsolete: true
This new patch makes the styleeditor tests pass.
It doesn't refactor what I commented about in the previous comment, but rather just makes this simple feature work.
I also added a new test for it.

I guess it makes sense to land something like this since it has immediate benefits, and then spend more time on the rest.

Heather: mind doing a first round of review on this?
Attachment #8422966 - Attachment is obsolete: true
Attachment #8423011 - Flags: review?(fayearthur)
Comment on attachment 8423011 [details] [diff] [review]
bug835896-better-selector-search v1.patch

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

Mainly small nits. Also, maybe I'm missing something, but when I search on this page (splinter review), I see 'dcamp@mozilla.com' in the autocomplete suggestions. http://i.imgur.com/Z0QSLmx.png.

::: browser/devtools/inspector/selector-search.js
@@ +402,3 @@
>      let total = 0;
>      let query = this.searchBox.value;
>      let toLowerCase = false;

get rid of 'toLowerCase' this since you remove its use with this patch.

@@ +501,3 @@
>          firstPart = "#" + firstPart;
>        }
> +      

whitespace

::: browser/devtools/inspector/test/browser_inspector_search-suggests-ids-and-classes.js
@@ +50,5 @@
> +    gBrowser.selectedBrowser.removeEventListener("load", onload, true);
> +    waitForFocus(setupTest, content);
> +  }, true);
> +
> +  content.location = "data:text/html," + 

one trailing whitespace.

@@ +102,5 @@
> +      let [key, suggestions] = keyStates[state];
> +      let actualSuggestions = popup.getItems();
> +      is(popup.isOpen ? actualSuggestions.length: 0, suggestions.length,
> +         "There are expected number of suggestions at " + state + "th step.");
> +      actualSuggestions = actualSuggestions.reverse();

reverse() is in-place so no need to reassign.

::: browser/devtools/sourceeditor/css-autocompleter.js
@@ +786,2 @@
>        }
> +      

whitespace

::: toolkit/devtools/server/actors/inspector.js
@@ +1475,5 @@
> +
> +      // Prefixing . or # in case it's a class or ID, to group results
> +      let firstA = a[0].substring(0, 1);
> +      let firstB = b[0].substring(0, 1);
> +      

whitespace

@@ +1476,5 @@
> +      // Prefixing . or # in case it's a class or ID, to group results
> +      let firstA = a[0].substring(0, 1);
> +      let firstB = b[0].substring(0, 1);
> +      
> +      if (firstA === "#") sortA = "2" + sortA;

This if/else style is really different from the rest of the file. The rest of the file looks like:

if (x) {
  // thing
}
else {
  // other thing
}

@@ +1479,5 @@
> +      
> +      if (firstA === "#") sortA = "2" + sortA;
> +      else if (firstA === ".") sortA = "1" + sortA;
> +      else sortA = "0" + sortA;
> +      

whitespace
Attachment #8423011 - Flags: review?(fayearthur) → review+
(In reply to Heather Arthur [:harth] from comment #28)
Thanks for the review Heather.

> Mainly small nits. Also, maybe I'm missing something, but when I search on
> this page (splinter review), I see 'dcamp@mozilla.com' in the autocomplete
> suggestions. http://i.imgur.com/Z0QSLmx.png.
wat? I don't even...
I'll investigate.

About all the extra whitespace problems, sorry, I just re-installed ST2 from scratch, and lost my 'remove-trailing-whitespaces-on-save' setting.
I will take care of those.
Setting R+ because this patch only has whitespace changes and the toLowercase line removed, as suggested.

(In reply to Heather Arthur [:harth] from comment #28)
> Comment on attachment 8423011 [details] [diff] [review]
> bug835896-better-selector-search v1.patch
> 
> Review of attachment 8423011 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Mainly small nits. Also, maybe I'm missing something, but when I search on
> this page (splinter review), I see 'dcamp@mozilla.com' in the autocomplete
> suggestions. http://i.imgur.com/Z0QSLmx.png.
Weird, I re-applied the patch again on a fresh fx-team, and I'm not seeing the problem (on the splinter page, logged in).
I tested with all letters and numbers, and I'm not seeing any email address part of the results: https://dl.dropboxusercontent.com/u/714210/search.png

Try build: https://tbpl.mozilla.org/?tree=Try&rev=da308f45f923
Attachment #8423011 - Attachment is obsolete: true
Attachment #8423737 - Flags: review+
Assignee: nobody → pbrosset
Status: NEW → ASSIGNED
OS: Windows 7 → All
Priority: -- → P2
Hardware: x86_64 → All
Green try build and R+.
This first patch is now in fx-team: https://hg.mozilla.org/integration/fx-team/rev/40f323c3c14e

Leaving this open to work on the text search.
Keywords: leave-open
Whiteboard: [fixed-in-fx-team]
https://hg.mozilla.org/mozilla-central/rev/40f323c3c14e
Flags: in-testsuite+
Whiteboard: [fixed-in-fx-team]
Posted patch simpler-search v1.patch (obsolete) — Splinter Review
Here is an attempt at a simpler/better search.

- Simpler in the sense that it doesn't require knowledge prior to using it, it works like you would expect a search field in a browser to work.
- Better because it finds things like tags, attribute names and values, text contents and comments.

It also still works with selectors as before.
The selector suggestions are gone though. We can of course add the suggestions popup back, but I'm not sure how it would feel from a UX standpoint. The popup would only list potential selectors whereas the field can accept anything.

Here is a screencast of the new search in action: http://quick.as/qmyasov0

Brian: we discussed about this earlier. What do you think?
Attachment #8425395 - Flags: feedback?(bgrinstead)
Note that attachment 8425395 [details] [diff] [review] doesn't work with iframes and when the user navigates to a new page with the toolbox opened yet. This would be easy to solve with bug 1007021.
Reporter

Comment 35

5 years ago
(In reply to Patrick Brosset [:pbrosset] [:patrick] from comment #34)
> Note that attachment 8425395 [details] [diff] [review] doesn't work with
> iframes and when the user navigates to a new page with the toolbox opened
> yet. This would be easy to solve with bug 1007021.

If inspector works with iframe changes and navigations, why is this new feature dependent on some other approach of getting/detecting navigation changes ?

(In reply to Patrick Brosset [:pbrosset] [:patrick] from comment #33)
> Created attachment 8425395 [details] [diff] [review]
> simpler-search v1.patch
> 
> Here is an attempt at a simpler/better search.
> 
> - Simpler in the sense that it doesn't require knowledge prior to using it,
> it works like you would expect a search field in a browser to work.
> - Better because it finds things like tags, attribute names and values, text
> contents and comments.

In the screenshot you used a search like "foo bar [id]" - Are we bringing in some new hidden syntax for the searches ?

> It also still works with selectors as before.

What happens when there is a mixture of selector and arbit text like "hello World.greenColor" ?


> The selector suggestions are gone though. We can of course add the

I think the selector suggestions was a very nic feature of giving a feedback while searching. Many a times the user is not sure of correct spellings used in classes and stuff. The suggestions help iterate on those issues very quickly.

Its still okay if the suggestions showed just selectors only (like introduced by the previous patch in this bug which is also checked in)
(In reply to Girish Sharma [:Optimizer] from comment #35)
> (In reply to Patrick Brosset [:pbrosset] [:patrick] from comment #34)
> > Note that attachment 8425395 [details] [diff] [review] doesn't work with
> > iframes and when the user navigates to a new page with the toolbox opened
> > yet. This would be easy to solve with bug 1007021.
> 
> If inspector works with iframe changes and navigations, why is this new
> feature dependent on some other approach of getting/detecting navigation
> changes ?
Because of the new class I introduced to index a document (the thing that lists attributes, tags, text nodes and comment nodes). It just receives a single document as argument and indexes what's in it.

> (In reply to Patrick Brosset [:pbrosset] [:patrick] from comment #33)
> > Created attachment 8425395 [details] [diff] [review]
> > simpler-search v1.patch
> > 
> > Here is an attempt at a simpler/better search.
> > 
> > - Simpler in the sense that it doesn't require knowledge prior to using it,
> > it works like you would expect a search field in a browser to work.
> > - Better because it finds things like tags, attribute names and values, text
> > contents and comments.
> 
> In the screenshot you used a search like "foo bar [id]" - Are we bringing in
> some new hidden syntax for the searches ?
This was just to show that css selectors are still supported. In fact everytime a search is made, I search both in the indexed data and try to execute a querySelectorAll with the query.

> > It also still works with selectors as before.
> 
> What happens when there is a mixture of selector and arbit text like "hello
> World.greenColor" ?
well, if "hello World.greenColor" matches nodes in the document (<hello><World class="greenColor">) then those nodes will be part of the results, as will the nodes that contain the text "hello World.greenColor" if any.
> 
> > The selector suggestions are gone though. We can of course add the
> 
> I think the selector suggestions was a very nic feature of giving a feedback
> while searching. Many a times the user is not sure of correct spellings used
> in classes and stuff. The suggestions help iterate on those issues very
> quickly.
I agree, the suggestions as you type thing is pretty nice.
> Its still okay if the suggestions showed just selectors only (like
> introduced by the previous patch in this bug which is also checked in)
We can experiment putting them back in with the new search, see how it feels. I just wanted to demo something quickly to get feedback so I didn't bother try to make the both work together, but it should be relatively easy to put back in.
Reporter

Comment 37

5 years ago
(In reply to Patrick Brosset [:pbrosset] [:patrick] from comment #36)
> (In reply to Girish Sharma [:Optimizer] from comment #35)
> > (In reply to Patrick Brosset [:pbrosset] [:patrick] from comment #34)
> > > Note that attachment 8425395 [details] [diff] [review] doesn't work with
> > > iframes and when the user navigates to a new page with the toolbox opened
> > > yet. This would be easy to solve with bug 1007021.
> > 
> > If inspector works with iframe changes and navigations, why is this new
> > feature dependent on some other approach of getting/detecting navigation
> > changes ?
> Because of the new class I introduced to index a document (the thing that
> lists attributes, tags, text nodes and comment nodes). It just receives a
> single document as argument and indexes what's in it.

Oh, then are we not tracking any changes made in the same document ? Like element additions etc ?
 
> > (In reply to Patrick Brosset [:pbrosset] [:patrick] from comment #33)
> > > Created attachment 8425395 [details] [diff] [review]
> > > simpler-search v1.patch
> > > 
> > > Here is an attempt at a simpler/better search.
> > > 
> > > - Simpler in the sense that it doesn't require knowledge prior to using it,
> > > it works like you would expect a search field in a browser to work.
> > > - Better because it finds things like tags, attribute names and values, text
> > > contents and comments.
> > 
> > In the screenshot you used a search like "foo bar [id]" - Are we bringing in
> > some new hidden syntax for the searches ?
> This was just to show that css selectors are still supported. In fact
> everytime a search is made, I search both in the indexed data and try to
> execute a querySelectorAll with the query.

so in most cases "foo bar [id]" would not result in anything, right ? querySelectorAll will simply try to think that its a selector for any element with id attribute inside of a bar tag inside of a foo tag, while on the other hand, user might get confused on why an element with no text like "foo bar [id]" coming up in search result .

Its really difficult to club these two UI's together IMO.

> > > It also still works with selectors as before.
> > 
> > What happens when there is a mixture of selector and arbit text like "hello
> > World.greenColor" ?
> well, if "hello World.greenColor" matches nodes in the document
> (<hello><World class="greenColor">) then those nodes will be part of the
> results, as will the nodes that contain the text "hello World.greenColor" if
> any.

Yeah, its confusing to user in a case when his intention is to search plain text.

 
> > > The selector suggestions are gone though. We can of course add the
> > 
> > I think the selector suggestions was a very nic feature of giving a feedback
> > while searching. Many a times the user is not sure of correct spellings used
> > in classes and stuff. The suggestions help iterate on those issues very
> > quickly.
> I agree, the suggestions as you type thing is pretty nice.
> > Its still okay if the suggestions showed just selectors only (like
> > introduced by the previous patch in this bug which is also checked in)
> We can experiment putting them back in with the new search, see how it
> feels. I just wanted to demo something quickly to get feedback so I didn't
> bother try to make the both work together, but it should be relatively easy
> to put back in.

Yeah, and I think that the suggestions to not have to bother about text search at all....
(In reply to Girish Sharma [:Optimizer] from comment #37)
> (In reply to Patrick Brosset [:pbrosset] [:patrick] from comment #36)
> > (In reply to Girish Sharma [:Optimizer] from comment #35)
> > > (In reply to Patrick Brosset [:pbrosset] [:patrick] from comment #34)
> > > > Note that attachment 8425395 [details] [diff] [review] doesn't work with
> > > > iframes and when the user navigates to a new page with the toolbox opened
> > > > yet. This would be easy to solve with bug 1007021.
> > > 
> > > If inspector works with iframe changes and navigations, why is this new
> > > feature dependent on some other approach of getting/detecting navigation
> > > changes ?
> > Because of the new class I introduced to index a document (the thing that
> > lists attributes, tags, text nodes and comment nodes). It just receives a
> > single document as argument and indexes what's in it.
> 
> Oh, then are we not tracking any changes made in the same document ? Like
> element additions etc ?
We are. See in the patch, there's a mutation observer used for that. We're just not going into iframes nor re-indexing when a document is destroyed and a new one created (navigation/reload).
Assignee

Comment 39

5 years ago
(In reply to Patrick Brosset [:pbrosset] [:patrick] from comment #36)
> (In reply to Girish Sharma [:Optimizer] from comment #35)
> > I think the selector suggestions was a very nic feature of giving a feedback
> > while searching. Many a times the user is not sure of correct spellings used
> > in classes and stuff. The suggestions help iterate on those issues very
> > quickly.
> I agree, the suggestions as you type thing is pretty nice.
> > Its still okay if the suggestions showed just selectors only (like
> > introduced by the previous patch in this bug which is also checked in)
> We can experiment putting them back in with the new search, see how it
> feels. I just wanted to demo something quickly to get feedback so I didn't
> bother try to make the both work together, but it should be relatively easy
> to put back in.

I agree that the suggestions can be nice, but it will be tricky to come up with a UX that makes sense for providing both full text search and selector autocompletion.  I think full text search is more important than selector autocompletion so IMO we should revisit the autocompletion as a follow up.
Assignee

Comment 40

5 years ago
Comment on attachment 8425395 [details] [diff] [review]
simpler-search v1.patch

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

I generally like the approach - left a few notes.  It may get quite a bit more complicated when adding iframe support, so I'm interested on what approach you are planning on taking for this.

::: browser/devtools/markupview/markup-view.js
@@ +1755,5 @@
> +  highlightPart: function(type, value, part) {
> +    // Make sure the highlight is done after update to avoid race conditions
> +    this.updating.then(() => {
> +      let range = this.doc.createRange();
> +      let textNode = this.value.firstChild;

Abstract the functionality  starting after this line into a function for reuse with ElementEditor highlightPart

@@ +1757,5 @@
> +    this.updating.then(() => {
> +      let range = this.doc.createRange();
> +      let textNode = this.value.firstChild;
> +
> +      let start = textNode.textContent.toLowerCase().indexOf(part.toLowerCase());

Since this happens after update(), should we check to make sure start is not -1 to prevent getting an error on the range?  In case the updated text is different from the searched text.

@@ +2107,5 @@
> +      // No need to highlight anything if the type is selector
> +      return;
> +    }
> +
> +    // FIXME: if the node isn't imported yet, we have a race condition and the

When I search for '900' on http://fiddle.jshell.net/bgrins/z8pXp/show/ I don't see any issue with non-imported nodes, but it could be that I'm just not hitting the condition.  Either way, would using ensureVisible() instead of waitForChildren() before calling highlightPart fix this?

::: toolkit/devtools/server/actors/document-search.js
@@ +138,5 @@
> + * s.destroy();
> + *
> + * @param {Document} doc The document to be searched
> + *
> + * XXX: Doesn't work with iframes and when page is reloaded.

For iframe support, I can think of a couple of things:

1) Could rename to PageSearch and have a setDocuments() call that makes a documentindex for each doc in the set.  The tricky part is that I'm not sure if the markup view is necessarily aware of all of the frames at all times (like if one hasn't been imported).
2) Or DocumentSearch could be the class is where the mutation observers should live, and keep track of all new documents that way (instead of relying on the markup view).  Making a new DocumentIndex for each one detected, and removing any that don't exist anymore.  So just keeping a of => { observer, docIndex, Set:nestedDocs } for each doc.  Then just set docIndex._doc = null on the observer change.

Probably also looking into how the WalkerActor handles nested docs and seeing if we could maybe use that in place of the treeWalker to solve this problem

For reloading could we just destroy the documentsearch from the markup view and construct a new one with this.rootDoc when a reload happens?

@@ +199,5 @@
> +    }
> +
> +    let nodes = [];
> +    try {
> +      nodes = [...this.doc.querySelectorAll(query)];

Should use CSS.escape(query) here to catch escaped selector characters
Attachment #8425395 - Flags: feedback?(bgrinstead) → feedback+
(In reply to Patrick Brosset [:pbrosset] [:patrick] from comment #31)
> Green try build and R+.
> This first patch is now in fx-team:
> https://hg.mozilla.org/integration/fx-team/rev/40f323c3c14e
> 
> Leaving this open to work on the text search.
This first patch is buggy. STR:
- open a bugzilla patch's review page
- switch the R flag select box to R?
- in the inspector search box, type "a " (a following by a space)
=> You should see something like this:
https://www.dropbox.com/s/anr9vfs4lu5syws/Screenshot%202014-05-20%2008.17.53.png
Depending on the outcome of the recent discussion about the selector suggestions, we might want or not to fix it...
I think there are 2 slightly different things that you might want when you're searching.

1. It's your page and you understand it, and you just want to get to foo quickly. In this case, I think what you've done fits really well, I think you could probably navigate anywhere very quickly on a page that you understand with this.

2. It's not your page, and you don't understand it, and you're trying to work out what's going on. I think there are 2 things that we're missing for this case: completion helps more here. If you press # you get a list of the ids on the page etc, but more a list of results would be really handy. Show me everywhere on this page that 'bar' exists, and let me skim to pick where to go.

My take is that we're not supporting 2 well at all right now, and we shouldn't worry about that in this bug, but perhaps we should have a search tool that unifies the search from all the panels, more along the lines of the search in Firebug or Chrome.
Reporter

Comment 43

5 years ago
I just noticed that the changes to the querySelectorAll in inspector.js has also affected style editor. When you type a class name without a . in style editor, the proper selector is suggested but the . is not inserted in the editor.
(In reply to Girish Sharma [:Optimizer] from comment #43)
> I just noticed that the changes to the querySelectorAll in inspector.js has
> also affected style editor. When you type a class name without a . in style
> editor, the proper selector is suggested but the . is not inserted in the
> editor.
Damn! I made sure the suggestions popup still worked in the styleeditor, but didn't see this bug coming.
It's a little bit unfortunate because this needs fixing on the client side of the editor (just like I had to fix it on the client side part of the inspector search). The getSuggestions method doesn't handle everything and there's therefore some code duplication between the panels that want to use it.
Anyway, according to our discussions on the mailing list (https://groups.google.com/forum/#!topic/mozilla.dev.developer-tools/7znRmRy3jGM), I think it'd be better to rollback this patch.
Keywords: leave-open

Comment 46

5 years ago
As we're all aware, the ultimate answer is 42... as in comment 42 ;)  Joe has hit the nail square on the head.

Let me add also, ...

The search should always provide predicable results ("least surprise") especially in Joe's scenario 1.  If I know the HTML and I search for a known sequence, it should be found.  I'm guessing that bug 967493 will affect results? (because the HTML pane contains a rendering (translation) of the HTML not the actual HTML sent down the wire).

Lastly, iframes.  Iframes, iframes, iframes. Only Chrome (in my experience) searches them in a sane and stable manner. Firebug drops them, or forgets to update them when changed dynamically. FF devtools can't search them.  From the tool user's perspective, it's just another block element with child elements (little more than a div with src attr). And if you can address (or consider) bug 969818 while you're there, great.

0.02
(In reply to Russ from comment #46)
> Lastly, iframes.  Iframes, iframes, iframes. Only Chrome (in my experience)
> searches them in a sane and stable manner. Firebug drops them, or forgets to
> update them when changed dynamically. FF devtools can't search them.  From
> the tool user's perspective, it's just another block element with child
> elements (little more than a div with src attr).
Good point, we definitely need to be able to search iframes. Even if my patch in comment 33 doesn't take them into account, I planned to do it and am working on bug 1007021 to make this happen easily.
Assignee

Updated

5 years ago
Blocks: 1026479
I don't have time to work on this right now, so here are some notes for anyone who may want to continue working on this:

- The search input box should be more like the debugger one:
  - provide 2 motes: search by free-text, search by selector
  - a prefix (! is used in the debugger for instance) could help trigger one mode or the other
  - a help popup that appears when focusing the empty field would help
- In free-text mode: the DocumentSearch API I added in attachment 8425395 [details] [diff] [review] can be used (it needs to make use of the multi-windows management landed in bug 1007021 to search in all frames though).
- In selector mode: we should keep exactly what we have right now: selector suggestions, but we should also fix that so it works across frames (thought that could be done as a separate bug).
- A new bug could be filed to also add a list of results instead of simply selecting the node that matches in the markup-view, so you can see all results in one place
Assignee: pbrosset → nobody

Comment 49

5 years ago
So what does the search string #mySomething mean across views/documents?  And .doStuff?

Seems to me this could get messy.  LEt's say I have an id, mySomething.  searching html I'd expect to say #mySomething to nail it down.  Would I do ##mySomething in some contexts? or just mySomething (and potentially return all classes, ids, functions, methods and variables that happen to use the same text (not to mention code carrying documentation about code that may mention it a hundred times).
Assignee

Comment 50

5 years ago
(In reply to Patrick Brosset [:pbrosset] [:patrick] from comment #48)
> I don't have time to work on this right now, so here are some notes for
> anyone who may want to continue working on this:
> 
> - The search input box should be more like the debugger one:
>   - provide 2 motes: search by free-text, search by selector
>   - a prefix (! is used in the debugger for instance) could help trigger one
> mode or the other
>   - a help popup that appears when focusing the empty field would help
> - In free-text mode: the DocumentSearch API I added in attachment 8425395 [details] [diff] [review]
> [details] [diff] [review] can be used (it needs to make use of the
> multi-windows management landed in bug 1007021 to search in all frames
> though).
> - In selector mode: we should keep exactly what we have right now: selector
> suggestions, but we should also fix that so it works across frames (thought
> that could be done as a separate bug).
> - A new bug could be filed to also add a list of results instead of simply
> selecting the node that matches in the markup-view, so you can see all
> results in one place

My takeaway regarding the 'modes' was a bit different.  The way I followed it was that any search query would try to match based on a number of criteria (selector, text content, attribute names/values, tag name, etc).  If a search started with a '.' or a '#', it would simply be a hint that it is probably a selector, and would trigger the autocomplete results.  However, this search would still match non-selector results.  So if you had markup like:

<div class="test">test</div>
<span id="foo">.test</span>

Then a search for ".test" would still match both elements (not just the one in which the selector matched).

If the query doesn't start with a '#' or '.', then all results (including a selector) would still match - it just wouldn't show autocomplete suggestions.  So if you searched for "span" here it would find the span, just like if you searched "span#foo" or even "spa" (not matching selector, but a partial match on tag name).

IIRC, the current patches here pretty much works this way already, with the exception of autocomplete listings on the textbox.
I don't have a big preference either way, I just thought using a symbol/character (or a radio button as we discussed at some stage) would be easier for the user to know what to expect from the search field. Mixing 2 result types and showing suggestions for selectors feels a little strange to me.
I'd say let's experiment.
Reporter

Comment 52

5 years ago
I agree that separating out the two behaviors based on some radio button/toggle would be better (IMO)

The placeholder can then also switch accordingly between "Search HTML" and "Search Selector" to better hint the current behavior.

We can then show suggestions only in the selector mode and treat symbols like "." and "#" with no extra meaning in the html (plain text) mode.
One downside of the toggle approach is that it introduces a mode. Other modes include Caps Lock and the "Match case" feature of some search boxes.

> Modes are often frowned upon in interface design because they are likely to
> produce mode errors when the user forgets what state the interface is in,
> performs an action that is appropriate to a different mode, and gets an
> unexpected and undesired response.
- http://en.wikipedia.org/wiki/Mode_%28computer_interface%29#Mode_errors
Reporter

Comment 54

5 years ago
So here we have a choice between introducing a lot of direct confusion with one approach and a tendency to forget and get confused in the other.

I'd still prefer mode instead of prefixes, as we can always have a popup on focus (like debugger) to remind user about the mode and give him the option to switch.
Assignee

Comment 55

5 years ago
(In reply to Girish Sharma [:Optimizer] from comment #54)
> So here we have a choice between introducing a lot of direct confusion with
> one approach and a tendency to forget and get confused in the other.

Based on the way you are framing this choice I don't think we should do either :).

Which approach causes a lot of direct confusion?  I'm assuming you mean the non-modal search, but I'm not clear why it is confusing.  You search for a string, and it matches anything in the full text of the page plus any node that matches the selector.  My original suggestion was to ditch the autocomplete suggestions over the textbox altogether, as it would simplify the UI and I don't think it is a widely used feature (even now as the only search option in the markup view).  I still think that is the best way to proceed, but I do agree with Patrick that we should experiment with it.
There may be other options worth exploring. If nothing else, usability would be improved if the mode was made visible at the locus of attention. For example, the search box could have different placeholder text or be colored differently depending on which mode was selected.
Reporter

Comment 57

5 years ago
(In reply to Brian Grinstead [:bgrins] from comment #55)
> (In reply to Girish Sharma [:Optimizer] from comment #54)
> > So here we have a choice between introducing a lot of direct confusion with
> > one approach and a tendency to forget and get confused in the other.
> 
> Based on the way you are framing this choice I don't think we should do
> either :).
> 
> Which approach causes a lot of direct confusion?  I'm assuming you mean the
> non-modal search, but I'm not clear why it is confusing.  You search for a
> string, and it matches anything in the full text of the page plus any node
> that matches the selector.  My original suggestion was to ditch the
> autocomplete suggestions over the textbox altogether, as it would simplify
> the UI and I don't think it is a widely used feature (even now as the only
> search option in the markup view).  I still think that is the best way to
> proceed, but I do agree with Patrick that we should experiment with it.

Yes, I was referring to the non mode approach to be directly confusing.

To power users, it will make sense, but otherwise, mixing a full text search with a selector based search can be confusing to a lot of people.

Regarding suggestions popup, I personally think that it is a very good discovery feature. Many a times the developer is unfamiliar with the webpage structure. So instead of starting out a char word full text search (which will result in infinitely matched items) and then going by hit and trial method to actually reach to the desired node, its a lot more helpful to get list of items present on the page.

A similar behavior is present in Debugger as well, for sources search, function search etc. In debugger, a special-char-to-separate modes works because those chars do not have any specific meaning of their own (unlike in inspector where . # mean something)

Comment 58

5 years ago
(In reply to Girish Sharma [:Optimizer] from comment #57)
> 
> Regarding suggestions popup, I personally think that it is a very good
> discovery feature. Many a times the developer is unfamiliar with the webpage
> structure. 

BINGO!  The tools often seem to assume at least partial code/resource awareness on the part of the dev-user. This is by no means a safe assumption. 

So instead of starting out a char word full text search (which
> will result in infinitely matched items) and then going by hit and trial
> method to actually reach to the desired node, its a lot more helpful to get
> list of items present on the page.
> 
> A similar behavior is present in Debugger as well, for sources search,
> function search etc. In debugger, a special-char-to-separate modes works
> because those chars do not have any specific meaning of their own (unlike in
> inspector where . # mean something)

Right. And also, being able to search html for *code* - something which I believe is impossible right now (haven't used the tools in a while).  It's no good if the code searching technology only works in the debugger:
<div...> ... <script ...> //this is code too</script> ... </div>

In reality, sources can be "mixed" regardless of how devtools chooses to slice them up for its own presentational reasons.  And this is frequently the case dealing with 3rd party sources - trying to figure out what piece of code is modifying a part of the UI/dom can be hard when it's buried in a bunch of unfamiliar HTML which isn't presented at all in the tools.

And when the HTML has injected iframes... well, you get the idea.
I just discovered the find method in the nsIDOMWindow interface, not sure if it can be useful, but it's worth checking out (available to chrome code only): http://mxr.mozilla.org/mozilla-central/source/dom/interfaces/base/nsIDOMWindow.idl#433
Duplicate of this bug: 1133959
Duplicate of this bug: 1133612
Assignee

Updated

4 years ago
Depends on: 1149346
Whiteboard: [devedition-40]
Whiteboard: [devedition-40] → [devedition-40][difficulty=hard]
Priority: P2 → P1
Assignee

Comment 62

4 years ago
I'm going to take this one unless if Patrick objects
Assignee: nobody → bgrinstead
Whiteboard: [devedition-40][difficulty=hard] → [devedition-40][difficulty=medium]
Assignee

Updated

4 years ago
Depends on: 873443
Assignee

Comment 63

4 years ago
Comment on attachment 8423737 [details] [diff] [review]
bug835896-better-selector-search v2.patch

Moved this to bug 1149346
Attachment #8423737 - Attachment is obsolete: true
Assignee

Updated

4 years ago
See Also: → 1153625
Assignee

Comment 64

4 years ago
Posted patch inspector-search-string.patch (obsolete) — Splinter Review
A new string, just in case we need to land it for 40.  I won't land this unless if it looks like the implementation won't make it in before merge day.
Attachment #8593100 - Flags: review?(pbrosset)
Assignee

Comment 65

4 years ago
Posted patch inspector-search-WIP.patch (obsolete) — Splinter Review
Rebased and changed the organization a bit.  In this patch I've kept the existing selector search in the file so it'll be easier to support both search types
Attachment #8425395 - Attachment is obsolete: true
Comment on attachment 8593100 [details] [diff] [review]
inspector-search-string.patch

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

::: browser/locales/en-US/chrome/browser/devtools/inspector.dtd
@@ +84,5 @@
>  
> +<!-- LOCALIZATION NOTE (inspectorSearchHTML.label3): This is the label that will
> +     be shown as the placeholder in inspector search box once the tool supports the
> +     full HTML search from Bug 835896. -->
> +<!ENTITY inspectorSearchHTML.label3          "Search HTML">

So, I know we're supposed to create new strings everytime, but shouldn't we get rid of lavel2?
Attachment #8593100 - Flags: review?(pbrosset) → review+
Assignee

Comment 67

4 years ago
(In reply to Patrick Brosset [:pbrosset] [:patrick] from comment #66)
> Comment on attachment 8593100 [details] [diff] [review]
> inspector-search-string.patch
> 
> Review of attachment 8593100 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: browser/locales/en-US/chrome/browser/devtools/inspector.dtd
> @@ +84,5 @@
> >  
> > +<!-- LOCALIZATION NOTE (inspectorSearchHTML.label3): This is the label that will
> > +     be shown as the placeholder in inspector search box once the tool supports the
> > +     full HTML search from Bug 835896. -->
> > +<!ENTITY inspectorSearchHTML.label3          "Search HTML">
> 
> So, I know we're supposed to create new strings everytime, but shouldn't we
> get rid of lavel2?

I'll only land this if it looks like the main patch isn't going to land in time for the merge, in which case we'd want to keep label2 around and remove it in the main patch.  Hopefully we won't need to do that though.
Assignee

Comment 68

4 years ago
Includes two extra strings in inspector.properties, for showing the current result and also a message when no results were found.
Attachment #8593100 - Attachment is obsolete: true
Attachment #8603022 - Flags: review+
Assignee

Comment 69

4 years ago
Landed strings

remote:   https://hg.mozilla.org/integration/fx-team/rev/c4b635e71477
Keywords: leave-open
Whiteboard: [devedition-40][difficulty=medium] → [fixed-in-fx-team][devedition-40][difficulty=medium]
https://hg.mozilla.org/mozilla-central/rev/c4b635e71477
Whiteboard: [fixed-in-fx-team][devedition-40][difficulty=medium] → [devedition-40][difficulty=medium]
Please don't use %S when there are multiple variables in a string
https://developer.mozilla.org/en-US/docs/Mozilla/Localization/Localization_content_best_practices#Use_ordered_variables_in_string_with_multiple_variables

A localizer could already swap "%S of %S" with "%1$S of %2$S", but not everybody is aware of that.
Assignee

Comment 72

4 years ago
(In reply to Francesco Lodolo [:flod] (UTC+2) from comment #71)
> Please don't use %S when there are multiple variables in a string
> https://developer.mozilla.org/en-US/docs/Mozilla/Localization/
> Localization_content_best_practices#Use_ordered_variables_in_string_with_mult
> iple_variables
> 
> A localizer could already swap "%S of %S" with "%1$S of %2$S", but not
> everybody is aware of that.

So I should update this string with a new version like so?

inspector.searchResultsCount2=%1$S of %2$S
Flags: needinfo?(francesco.lodolo)
(In reply to Brian Grinstead [:bgrins] from comment #72)
> So I should update this string with a new version like so?
> 
> inspector.searchResultsCount2=%1$S of %2$S

Exactly, and explain in the comment what %1$S and %2$S are.
Flags: needinfo?(francesco.lodolo)
Assignee

Comment 74

4 years ago
Attachment #8603864 - Flags: review?(francesco.lodolo)
Comment on attachment 8603864 [details] [diff] [review]
inspector-string-update.patch

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

LGTM, thanks.

::: browser/locales/en-US/chrome/browser/devtools/inspector.properties
@@ +74,5 @@
>  inspector.expandPane=Expand pane
>  
>  # LOCALIZATION NOTE (inspector.searchResultsCount): This is the label that
> +# will show up next to the inspector search box. %1$S is the current result
> +# index and %2$S is the total number of search results.  For example, "3 of 9".

nit: remove double space, colon after example.

… search results. For example: "3 of 9".
Attachment #8603864 - Flags: review?(francesco.lodolo) → review+
Assignee

Comment 76

4 years ago
Pushed the updated string with fix from Comment 75

remote:   https://hg.mozilla.org/integration/fx-team/rev/531d5a989027
Whiteboard: [devedition-40][difficulty=medium] → [fixed-in-fx-team][devedition-40][difficulty=medium]
https://hg.mozilla.org/mozilla-central/rev/531d5a989027
Whiteboard: [fixed-in-fx-team][devedition-40][difficulty=medium] → [devedition-40][difficulty=medium]
Assignee

Updated

4 years ago
Attachment #8603022 - Flags: checkin+
Assignee

Updated

4 years ago
Attachment #8603864 - Flags: checkin+
Assignee

Updated

4 years ago
Whiteboard: [devedition-40][difficulty=medium] → [devedition-40][difficulty=hard]
Whiteboard: [devedition-40][difficulty=hard] → [polish-backlog][difficulty=hard]
Marking this with the [devtools-ux] flag. The important ux-related discussion was about finding a way to provide both full text search and css selector autocompletion in the same search field.
Whiteboard: [polish-backlog][difficulty=hard] → [polish-backlog][difficulty=hard][devtools-ux]
Assignee

Comment 79

4 years ago
Posted patch inspector-search-WIP.patch (obsolete) — Splinter Review
Rebased
Attachment #8593519 - Attachment is obsolete: true
(In reply to Panos Astithas [:past] from comment #7)
> If this searchbox were visually confined to the style inspector, I would
> agree. As it is now, however, it almost looks like it targets the markup
> panel (it's right above it!), in which case searching for anything inside
> the markup would be the expected thing for the uninitiated. I would argue
> that searching for CSS selectors, although extremely useful, should be
> hidden behind an operator, like we do in the debugger. 

Super agree with Panos that the search tool looks like it's related to the markup panel since it's right above it, and having it target CSS selectors is confusing. 

I think it makes the most sense for the Inspector search bar to highlight the instances of words (whether or not they're in a tag or not). So to clarify:

> <button>Hey, I'm a button!</button>

would return three instances of the word (the beginning tag, the instance of "button" in the text string, and the end tag). I think that there could be an argument for only returning two instances and ignoring ending tags, since they're sort of redundant, but maybe y'alls can wrack your brains for an instance where someone might want/need that?

I also think it would be a good enhancement to highlight the word itself, not the line it occurred on (since highlighting a line with three instances on it isn't as useful as highlighting the word itself). So that's something additional to what Brian's doing in his patch, I think.
(In reply to Helen V. Holmes (:helenvholmes) (:✨) from comment #80)
> (In reply to Panos Astithas [:past] from comment #7)
...
> I think it makes the most sense for the Inspector search bar to highlight
> the instances of words (whether or not they're in a tag or not). So to
> clarify:
> 
> > <button>Hey, I'm a button!</button>
> 
> would return three instances of the word (the beginning tag, the instance of
> "button" in the text string, and the end tag). I think that there could be
> an argument for only returning two instances and ignoring ending tags, since
> they're sort of redundant, but maybe y'alls can wrack your brains for an
> instance where someone might want/need that?

It depends. For your example, it's on the same line, seems redundant. For the use case of:

<div id="outer-wrap">
...100 lines of content
</div>

It might be useful to highlight the closing tag because it's so far away from the opening tag.

Aside: some tools have a nice / somewhat hidden feature of being able to jump back and forth between an opening and closing tag. This is a really convenient power user feature, whereas the idea of simply highlighting the closing tag no matter what and being able to jump to it as the next result is a more generally accessible feature.

> I also think it would be a good enhancement to highlight the word itself,
> not the line it occurred on (since highlighting a line with three instances
> on it isn't as useful as highlighting the word itself). So that's something
> additional to what Brian's doing in his patch, I think.

+1.
Assignee

Updated

4 years ago
Depends on: 1157048
Assignee

Comment 82

4 years ago
Bug 835896 - Inspector searchbox should also provide a way to search plain text on the page;r=pbrosset
Attachment #8666994 - Flags: review?(pbrosset)
Assignee

Updated

4 years ago
Attachment #8662070 - Attachment is obsolete: true
Assignee

Comment 83

4 years ago
(In reply to Brian Grinstead [:bgrins] from comment #82)
> Created attachment 8666994 [details]
> MozReview Request: Bug 835896 - Inspector searchbox should also provide a
> way to search plain text on the page;r=pbrosset
> 
> Bug 835896 - Inspector searchbox should also provide a way to search plain
> text on the page;r=pbrosset

Note: the inspector-panel b-c search tests are all still failing, but I'm trying to port *most* of the important search related tests into test_inspector-search.html since it's easier to deal with and a faster test to run.  I think the design of the implementation has stabilized enough that I'm ready for a first pass of feedback.
Comment on attachment 8666994 [details]
MozReview Request: Bug 835896 - Inspector searchbox should also provide a way to search plain text on the page;r=pbrosset

https://reviewboard.mozilla.org/r/20507/#review19301

Very nice work. Most of the code is easy to read and I don't have many comments (I had written some of it a long time ago and I can still read it, yay!).

I've noticed this while using the search box locally:
- On about:home, type ".c" in the box and press the arrow UP key. This makes the input box loose focus and it collapses. This does not happen currently.
- I noticed a small CSS problem when there are errors. On data:text/html,<style>div::before{content:"div"}</style><div>  enter ".c" and hit ENTER, the input border changes to red and I can see the input height changing slightly (1px I think). If I then delete ".c" the input's border returns to its normal state and I can see the input go back to its previous size (note that I'm on windows 10).

::: devtools/client/framework/selection.js:166
(Diff revision 1)
> +    if (value && value._parent && value._parent.singleTextChild == value) {

Can you use the public method parentNode() instead of this private property?

::: devtools/client/inspector/inspector-panel.js:313
(Diff revision 1)
> +    this.searchLabel = this.searchBox.ownerDocument.createElement("box");

I have some concerns about building the label UI here. It feels a bit lost in this huge file. 

First of all, I think `searchLabel` isn't necessarily the best name (it took me a while to figure out what it was doing), maybe `searchResultsLabel`.

Secondly, I think the `<box>` could be part of the XUL document already so it doesn't need to be created here, and just toggled via CSS when needed.

Thirdly: do you think it would make sense for SelectorSearch to be responsible for the code that toggles and sets the value? It would have the advantage of getting rid of the event listeners and `_updateSearchLabel` callback in this file.

::: devtools/client/inspector/inspector-panel.js:316
(Diff revision 1)
> +    this._updateSearchLabel = this._updateSearchLabel.bind(this);

This made me realize that we bind functions all over the place in this file. I like it when we do this only in the constructor, at least they're all grouped together and the code is easier to read/shorter this way.

If you have the time, could you please clean this up? (feel free to ignore as not directly related to this bug).

::: devtools/client/inspector/selector-search.js:30
(Diff revision 1)
> +function SelectorSearch(inspector, input) {

Let's take this opportunity to rename this class. SelectorSearch doesn't search for selectors anymore, but for full text. So the name should reflect this.

::: devtools/client/inspector/selector-search.js:61
(Diff revision 1)
> +    this._lastQuery = this.doFullTextSearch(this.searchBox.value, reverse);

The promise returned by `doFullTextSearch` needs to be handled in case there are errors.

`this._lastQuery = this.doFullTextSearch(this.searchBox.value, reverse).catch(e => console.error(e));

::: devtools/client/inspector/selector-search.js:80
(Diff revision 1)
> +      reverse: reverse

Could be shorter as:
`let res = yield this.walker.search(query, {reverse});

::: devtools/client/inspector/selector-search.js:385
(Diff revision 1)
>    _showPopup: function(aList, aFirstPart, aState) {

Could you take this opportunity to convert all `aArg` to `arg`? We've removed a lot of them in other parts of the code already.

::: devtools/client/inspector/selector-search.js:390
(Diff revision 1)
> -    for (let [value, count, state] of aList) {
> +    for (let [value, /*count*/, state] of aList) {

Or maybe `let [value, , state] of aList`
But that's up to you because it's not really better either.

::: devtools/server/actors/inspector.js:1130
(Diff revision 1)
> +  metadata: "array:json"

Let's leave it in for now (and hence rephrase the comment). I agree that we'll need it for highlighting matches later.

::: devtools/server/actors/inspector.js:2080
(Diff revision 1)
> +    let nodeList = new NodeListActor(this, results.map(r=>r.node));

s/r=>r.node/r => r.node

::: devtools/server/actors/inspector.js:2861
(Diff revision 1)
> +    events.emit(this, "any-mutation");

Nice, very useful to avoid other parts of our server code having to register their own mutation observers (which might not always be possible, but still)

::: devtools/server/actors/inspector.js:3354
(Diff revision 1)
> +    let metaData = { };

This variable isn't used yet. You probably have plans to use it later, but for now, I think this variable should be removed.

::: devtools/server/actors/inspector.js:3972
(Diff revision 1)
> +    if (!node)

nit: please use {} around the if body

::: devtools/server/actors/utils/walker-search.js:23
(Diff revision 1)
> + * let index = new WalkerIndex(doc);

Please update this comment, `doc` isn't the parameter anymore.

::: devtools/server/actors/utils/walker-search.js:24
(Diff revision 1)
> + * index.data; // returns the indexed data as a Map of with strings as keys

I think a word is missing in this sentence.
"a Map of ... with strings as keys"

::: devtools/server/actors/utils/walker-search.js:119
(Diff revision 1)
> + * let s = new WalkerSearch(doc);

Please update this documentation, `WalkerSearch` doesn't take `doc` as parameter anymore.

::: devtools/server/actors/utils/walker-search.js:159
(Diff revision 1)
> +      matches.push({

nit: `matches.push({type});

::: devtools/server/actors/utils/walker-search.js:167
(Diff revision 1)
> +      if (options.searchMethod(query, matched)) {

Maybe invert condition and use `continue` to reduce nesting in this function.

::: devtools/server/tests/mochitest/inspector-search-data.html:50
(Diff revision 1)
> +  <div class="
Attachment #8666994 - Flags: review?(pbrosset)
Oh noes, reviewboard has crashed somewhere in the middle. My review comments haven't all been copied over to bugzilla.
It looks like it failed when trying to print out the emoji in the test file.
(In reply to Patrick Brosset [:pbrosset] [:pbro] from comment #85)
> Oh noes, reviewboard has crashed somewhere in the middle. My review comments
> haven't all been copied over to bugzilla.
> It looks like it failed when trying to print out the emoji in the test file.
You can go to reviewboard to see all my comments (I filed bug 1212310 for this reviewboard bug).
Assignee

Comment 87

4 years ago
Comment on attachment 8666994 [details]
MozReview Request: Bug 835896 - Inspector searchbox should also provide a way to search plain text on the page;r=pbrosset

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/20507/diff/1-2/
Attachment #8666994 - Flags: review?(pbrosset)
Assignee

Comment 88

4 years ago
(In reply to Brian Grinstead [:bgrins] from comment #87)
> Comment on attachment 8666994 [details]
> MozReview Request: Bug 835896 - Inspector searchbox should also provide a
> way to search plain text on the page;r=pbrosset
> 
> Review request updated; see interdiff:
> https://reviewboard.mozilla.org/r/20507/diff/1-2/

I believe that this addresses all of your original comments.  I still have at least one more test to fix (devtools/client/inspector/test/browser_inspector_search-01.js).
Assignee

Comment 89

4 years ago
(In reply to Patrick Brosset [:pbrosset] [:pbro] from comment #84)
> Comment on attachment 8666994 [details]
> MozReview Request: Bug 835896 - Inspector searchbox should also provide a
> way to search plain text on the page;r=pbrosset
> 
> https://reviewboard.mozilla.org/r/20507/#review19301
> 
> Very nice work. Most of the code is easy to read and I don't have many
> comments (I had written some of it a long time ago and I can still read it,
> yay!).
> 
> I've noticed this while using the search box locally:
> - On about:home, type ".c" in the box and press the arrow UP key. This makes
> the input box loose focus and it collapses. This does not happen currently.
> - I noticed a small CSS problem when there are errors. On
> data:text/html,<style>div::before{content:"div"}</style><div>  enter ".c"
> and hit ENTER, the input border changes to red and I can see the input
> height changing slightly (1px I think). If I then delete ".c" the input's
> border returns to its normal state and I can see the input go back to its
> previous size (note that I'm on windows 10).
> 

These issues should be fixed.

> ::: devtools/client/inspector/inspector-panel.js:313
> (Diff revision 1)
> > +    this.searchLabel = this.searchBox.ownerDocument.createElement("box");
> 
> I have some concerns about building the label UI here. It feels a bit lost
> in this huge file. 
> 
> First of all, I think `searchLabel` isn't necessarily the best name (it took
> me a while to figure out what it was doing), maybe `searchResultsLabel`.

Sure, I forgot to do this in the last push but will make that change in the next one.

> Secondly, I think the `<box>` could be part of the XUL document already so
> it doesn't need to be created here, and just toggled via CSS when needed.

Done

> Thirdly: do you think it would make sense for SelectorSearch to be
> responsible for the code that toggles and sets the value? It would have the
> advantage of getting rid of the event listeners and `_updateSearchLabel`
> callback in this file.

Maybe, although then the search widget has to deal with localized strings.  I sort of like the separation at first glance, although it would get rid of some code if we made that move so I'll take a closer look after wrapping up test fixes.
Comment on attachment 8666994 [details]
MozReview Request: Bug 835896 - Inspector searchbox should also provide a way to search plain text on the page;r=pbrosset

https://reviewboard.mozilla.org/r/20507/#review22015

Thanks, I see all my comments have been addressed. I've tested locally and it seems to work really fine.
I found one problem though: the search doesn't work anymore in the browser toolbox with this patch applied. The search suggestions work fine, but pressing enter doesn't select elements in the inspector, and I haven't seen any errors in the console.

::: devtools/client/inspector/selector-search.js:62
(Diff revisions 1 - 2)
> +                           catch(e => console.error(e));

nit: we usually put the dot `.` on the second line in these cases (with the `catch`).

::: devtools/client/locales/en-US/inspector.dtd:117
(Diff revision 2)
> -     be shown as the placeholder in the future, once the inspector search box
> +     is shown as the placeholder for the markup view search in the inspector. -->

s/that will is shown/that is shown
Attachment #8666994 - Flags: review?(pbrosset)
Assignee

Updated

4 years ago
Attachment #8666994 - Flags: review?(pbrosset)
Assignee

Comment 91

4 years ago
Comment on attachment 8666994 [details]
MozReview Request: Bug 835896 - Inspector searchbox should also provide a way to search plain text on the page;r=pbrosset

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/20507/diff/2-3/
Attachment #8666994 - Flags: review?(pbrosset) → review+
Comment on attachment 8666994 [details]
MozReview Request: Bug 835896 - Inspector searchbox should also provide a way to search plain text on the page;r=pbrosset

https://reviewboard.mozilla.org/r/20507/#review22169

Looks good to me. 
Thanks for taking care of the browser toolbox issue.
Assignee

Comment 94

4 years ago
Posted file search-page.html
Everything else is ready except I'm getting bit by an intermittent here where the sort ordering for two nodes in different iframes occasionally returns the wrong thing (dt4 on osx 10.6 opt here: https://treeherder.mozilla.org/#/jobs?repo=try&revision=09655c325a5f).

I've attached a test page that demonstrates the same issue.  I feel that maybe this is the same issue as Bug 486002 - specifically Comment 3:  "In other words, if the nodes are disconnected, return either 0x23 or 0x25 depending on which root node's pointer is greater.  I wish I were kidding.  I wish even more that this weren't 100% supported by the old DOM Level 3 Core spec:".

Thinking we somehow need to look at frame ordering in this case instead
Assignee

Comment 95

4 years ago
I think I've got an easy fix for this problem.. new try push: https://treeherder.mozilla.org/#/jobs?repo=try&revision=3fd98488b9b9
Assignee

Updated

4 years ago
Status: ASSIGNED → RESOLVED
Last Resolved: 4 years ago
Keywords: leave-open
Resolution: --- → FIXED
See Also: → 1224473

Updated

4 years ago
Depends on: 1225459

Updated

4 years ago
Depends on: 1225514
It looks like the 'Target' field of this bug should be filled.

Sebastian
Assignee

Updated

4 years ago
Target Milestone: --- → Firefox 45
Assignee

Comment 99

4 years ago
Release Note Request (optional, but appreciated)
[Why is this notable]: Inspector searchbox now finds any content on the page.  It used to only search for nodes matching a css selector
[Suggested wording]: Inspector search now matches results from all content in the page, including subframes
[Links (documentation, blog post, etc)]:
relnote-firefox: --- → ?
Added to the release notes with Brian's wording.
Assignee

Comment 102

4 years ago
(In reply to Will Bamberg [:wbamberg] from comment #101)
> I've added a bit on this:
> https://developer.mozilla.org/en-US/docs/Tools/Page_Inspector/How_to/
> Examine_and_edit_HTML#Searching

This is great, thanks!
Flags: needinfo?(bgrinstead)

Updated

3 years ago
Depends on: 1253868
Assignee

Updated

3 years ago
Depends on: 1256658

Comment 103

3 years ago
Should also depend on https://bugzilla.mozilla.org/show_bug.cgi?id=967493 but that's just me, what do I know.  Seems crazy to me that all the docs and bugs talk about "displaying the markup" when entities, *all* of them, are completely missed.

0.02

Updated

3 years ago
Depends on: 1258569

Updated

3 years ago
Depends on: 1258570, 1258571
Assignee

Updated

3 years ago
Blocks: 1259060

Updated

3 years ago
Depends on: 1259390

Updated

3 years ago
Depends on: 1229773

Updated

2 years ago
Depends on: 1327141

Updated

2 years ago
Depends on: 1327740

Updated

2 years ago
Depends on: 1327760

Updated

2 years ago
Depends on: 1328006
Depends on: 1327037
Depends on: 1420153

Updated

11 months ago
Product: Firefox → DevTools
No longer depends on: 1327037
Regressions: 1327037
You need to log in before you can comment on or make changes to this bug.