Closed Bug 1219920 Opened 9 years ago Closed 8 years ago

<use> tag with className breaks suggestions from "Search with CSS selectors"

Categories

(DevTools :: Inspector, defect)

defect
Not set
normal

Tracking

(firefox45 affected, firefox46 fixed)

RESOLVED FIXED
Firefox 46
Tracking Status
firefox45 --- affected
firefox46 --- fixed

People

(Reporter: arni2033, Assigned: abdullams, Mentored)

Details

(Whiteboard: [good first bug][lang=js][polish-backlog])

Attachments

(3 files, 3 obsolete files)

STR:   (Win7_64, Nightly 45, 32bit, ID 20151029045227, new profile)
1. Open attached "testcase 1"
2. Open devtools -> Inspector, click "Search with CSS selectors" field
3. Type ".te" without quotes
4. Delete everything in "Search with CSS selectors" field
5. Delete <svg>
6. Type ".te" without quotes in "Search with CSS selectors" field

Result:       No suggestions appear after Step 3. One suggestion appears after Step 6.
Expectations: After Step 3 list of suggestions should appear

Note: I encountered it on a page with embedded video from youtube. Btw, if I delete
      only [class] attribute of <use> in step 5, it fixes suggestions as well.
Attachment #8680869 - Attachment filename: testcase 1 - {use} attribute with className breaks suggestions.html → testcase 1 - {use} with className breaks suggestions.html
Thanks for the detailed test case and error message, this helps.
Indeed, SVG elements' classNames are not Strings like for HTML elements. They are SVGAnimatedString [1].

In the function WalkerActor.getSuggestionsForQuery (in devtools/server/actors/inspector.js:2085)
We do `node.className.split(" ")` which doesn't work if `node.className` isn't a String (this is done in 2 places in the function, so whatever fix we do should be done in a helper function that is called from these 2 locations).

I'm pretty sure we already fixed something very similar, but in the webconsole, so we should do some research first to see if we can extract a helper in a shared place to be reused.

[1] https://developer.mozilla.org/en-US/docs/Web/API/SVGAnimatedString
Mentor: pbrosset
Whiteboard: [good first bug][lang=js]
Whiteboard: [good first bug][lang=js] → [good first bug][lang=js][polish-backlog]
Hello I wold like to be assigned this bug please
(In reply to Abdulla Alkaabi from comment #3)
> Hello I wold like to be assigned this bug please
I see you're new to bugzilla, so I guess this will be your first patch.
Before getting assigned to the bug, you should probably go through the dev environment doc, set everything up and start investigating the issue, to confirm this is a good fit for you.
https://wiki.mozilla.org/DevTools/GetInvolved
https://wiki.mozilla.org/DevTools/Hacking
Feel free to ask questions about the code, and information about where to get started (and make sure you check the "need info" box and put my name in when you do, so I am notified automatically).
Hello Patrick,

Thanks for offering help. I just setup what's needed but quite lost in finding which file to look at it here ?

Sorry if it's silly question but any help is appreciated.
Thanks.
Flags: needinfo?(pbrosset)
Also Patrick you said there is a similar fix in webconsole , where exactly is that , I want to check it out ?
Attached patch Patch (obsolete) — Splinter Review
That is my attempt on it
(In reply to Abdulla Alkaabi from comment #6)
> Also Patrick you said there is a similar fix in webconsole , where exactly
> is that , I want to check it out ?
Yes, but I don't think we can reuse it after all. Bug 1016830 was filed about this, and this got eventually fixed in bug 920141 (in Console.jsm we do something where if an element.className doesn't support the split function, we just return the empty string).

So anyway, here the problem is in file /devtools/server/actors/inspector.js, inside the getSuggestionsForQuery method.
In 2 places in this method we do `node.className.split(" ")`
node, here, can be any DOM nodes, and we know that if it's an SVG element, then node.className isn't a String.
Also this split(" ") isn't great because there might be more than 1 space separating classes.
So we want to replace these 2 calls with something better.

I'm wondering if we couldn't just use node.classList for this.

for (let className of node.classList) {...}
should work fine. Can you test this?

Also, the patch you uploaded doesn't seem correct. The diff [1] doesn't show all the changes you've made.
Please read through this doc [2] or this newer version [3]

[1] https://bugzilla.mozilla.org/attachment.cgi?id=8695514&action=diff
[2] https://developer.mozilla.org/en-US/docs/Mercurial/Using_Mercurial#How_can_I_generate_a_patch_for_somebody_else_to_check-in_for_me.3F
[3] http://mozilla-version-control-tools.readthedocs.org/en/latest/hgmozilla/index.html
Flags: needinfo?(pbrosset)
Attached patch Patch as suggested (obsolete) — Splinter Review
I hope that solve the problem. It seemed working for me. Anyone feel free to test it and send me feedback
Attachment #8695514 - Attachment is obsolete: true
This seems to work fine for me, but the patch doesn't apply cleanly for me, I don't know what happened when you created it. I had to redo the changes manually myself to test it locally.
Can you please take a look at the info at the end of comment 8, I think there was a problem when you generated the patch file.
Attachment #8697139 - Attachment description: Patch correctly generated according to → Patch correctly generated according to https://developer.mozilla.org/en-US/docs/Mercurial/Using_Mercurial#How_can_I_generate_a_patch_for_somebody_else_to_check-in_for_me.3F
Comment on attachment 8697139 [details] [diff] [review]
Patch correctly generated according to
https://developer.mozilla.org/en-US/docs/Mercurial/Using_Mercurial#How_can_I_generate_a_patch_for_somebody_else_to_check-in_for_me.3F

Flagging this for review so I remember to take a look at it in the coming days.
Attachment #8697139 - Flags: review?(pbrosset)
Attachment #8696539 - Attachment is obsolete: true
Comment on attachment 8697139 [details] [diff] [review]
Patch correctly generated according to
https://developer.mozilla.org/en-US/docs/Mercurial/Using_Mercurial#How_can_I_generate_a_patch_for_somebody_else_to_check-in_for_me.3F

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

This looks good, thanks.
A couple of last things are missing now:

- make sure the commit message is formatted correctly, it should be something like:
Bug 1219920 - Iterate through classNames in the WalkerActor using classList; r=pbro
If you are using Mercurial MQ, then you can change it simply with 'hg qref -m "Bug 1219920 - Iterate through classNames in the WalkerActor using classList; r=pbro"'
If you're using Mercurial but not MQ, you can change it using the histedit extension.
I've done it for you and will upload the patch.

- We need to add a test to make sure this bug doesn't come back again in the future.
We already have several tests related to the inspector search feature here: \devtools\client\inspector\test\browser_inspector_search-*.js
I suggest we create a new one that uses a new test HTML page that contains SVG.
Writing tests is hard and not the most interesting thing either, so since this is your first patch, I'll write the test myself. Please take a look at it, see if you understand it, and feel free to ask questions.
Attachment #8697139 - Flags: review?(pbrosset)
Assignee: nobody → abdullams
Status: NEW → ASSIGNED
Try build is green (apart from unrelated failures for known intermittents). So flagging this as "checkin-needed".
Thanks Abdulla for the patch. This should go in fairly soon and will be part of FF46.
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/138d994d5ba3
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 46
Has STR: --- → yes
[bugday-20160323]

Status: RESOLVED,FIXED -> UNVERIFIED

Comments:
STR:
Please make it more clear.
I am somewhat confused at "Select with CSS selectors". I haven't found any field named as such in
Inspector section.

Component: 
Name			Firefox
Version			46.0b9
Build ID		20160322075646
Update Channel	        beta
User Agent 		Mozilla/5.0 (Windows NT 6.1; WOW64; rv:46.0) Gecko/20100101 Firefox/46.0
OS			Windows 7 SP1 x86_64

Expected Results: 
Need more clear steps please. 

Actual Results: 
As expected
Flags: needinfo?(arni2033)
Yes, those STR are outdated, because that text label was changed to "Search HTML" in bug 835896.
It's just the search field above the markup, that's it.
Flags: needinfo?(arni2033)
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.