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)
DevTools
Inspector
Tracking
(firefox45 affected, firefox46 fixed)
RESOLVED
FIXED
Firefox 46
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
Comment 2•9 years ago
|
||
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]
Updated•8 years ago
|
Whiteboard: [good first bug][lang=js] → [good first bug][lang=js][polish-backlog]
Assignee | ||
Comment 3•8 years ago
|
||
Hello I wold like to be assigned this bug please
Comment 4•8 years ago
|
||
(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).
Assignee | ||
Comment 5•8 years ago
|
||
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)
Assignee | ||
Comment 6•8 years ago
|
||
Also Patrick you said there is a similar fix in webconsole , where exactly is that , I want to check it out ?
Assignee | ||
Comment 7•8 years ago
|
||
That is my attempt on it
Comment 8•8 years ago
|
||
(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)
Assignee | ||
Comment 9•8 years ago
|
||
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
Comment 10•8 years ago
|
||
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.
Assignee | ||
Comment 11•8 years ago
|
||
Assignee | ||
Updated•8 years ago
|
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 12•8 years ago
|
||
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)
Updated•8 years ago
|
Attachment #8696539 -
Attachment is obsolete: true
Comment 13•8 years ago
|
||
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)
Comment 14•8 years ago
|
||
Attachment #8697139 -
Attachment is obsolete: true
Attachment #8698967 -
Flags: review+
Updated•8 years ago
|
Assignee: nobody → abdullams
Status: NEW → ASSIGNED
Comment 15•8 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=5fd8f15df7ed&selectedJob=14653451
Comment 16•8 years ago
|
||
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
Comment 17•8 years ago
|
||
https://hg.mozilla.org/integration/fx-team/rev/138d994d5ba3
Keywords: checkin-needed
Comment 18•8 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/138d994d5ba3
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
status-firefox46:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 46
Comment 19•8 years ago
|
||
[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)
Reporter | ||
Comment 20•8 years ago
|
||
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)
Updated•6 years ago
|
Product: Firefox → DevTools
You need to log in
before you can comment on or make changes to this bug.
Description
•