Closed Bug 1360132 Opened 7 years ago Closed 7 years ago

escape tagName in findCssSelector

Categories

(DevTools :: Inspector, enhancement, P3)

Product:
DevTools
Component:
Inspector
Version:
51 Branch
Type:
enhancement

Tracking

(Not tracked)

Status:
RESOLVED FIXED
Milestone:
Firefox 55

People

(Reporter: kernp25, Assigned: nbeltran14, Mentored)

Details

(Keywords: good-first-bug)

Attachments

(2 files)

bug1360132 - 'Escaped tagName with cssEscape in findCssSelector to deal with special chars.'
59 bytes, text/x-review-board-request
pbro
: review+
Details
Bug 1360132 - Escaped tagName with cssEscape in findCssSelector to deal with special chars;
59 bytes, text/x-review-board-request
pbro
: review+
Details 
The tagName [1] also can contain ":" so it must be escaped (using CSS.escape)

[1] https://dxr.mozilla.org/mozilla-central/source/devtools/shared/inspector/css-logic.js#421
Component: Developer Tools → Developer Tools: Inspector
This sounds like an easy bug for someone who wants to try fixing their first DevTools bug.
Please read through the contribution guide first: https://developer.mozilla.org/en-US/docs/Tools/Contributing

Once you have the code, feel free to post a patch here, or comment for questions if needed (please use the needinfo? box at the bottom, with my name to make sure I see the question).

In terms of code, as said in comment 0, the file to be changed is /devtools/shared/inspector/css-logic.js.
The function in this file is findCssSelector.
Near the bottom of this file, the tagName of the element is appended to the selector string, but as noted above, we should make that part of the string safe by escaping it with CSS.escape first.
Mentor: pbrosset
Status: UNCONFIRMED → NEW
Ever confirmed: true
Keywords: good-first-bug
Priority: -- → P3
Hello. I was wondering if I might try to fix this bug. I've not tried fixing a bug yet, and it was mentioned that this would be a good bug for a beginner.
(In reply to nbeltran14 from comment #2)
> Hello. I was wondering if I might try to fix this bug. I've not tried fixing
> a bug yet, and it was mentioned that this would be a good bug for a beginner.

Replaying to my own comment because I forgot to use the "Need more information" box below in my earlier comment. I didn't see a way to check the box for my comment after I sent it. My apologies. Being new, I'm trying to get used to the flow of doing things.
Flags: needinfo?(pbrosset)
Sure, thanks for wanting to fix this.
I'll hold the bug for you. Comment 1 should contain enough information for you to get started, but please let me know if you need help.
Flags: needinfo?(pbrosset)
Assignee: nobody → nbeltran14
Status: NEW → ASSIGNED
Under Submitting the Patch on the How to submit a patch page (https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/How_to_Submit_a_Patch), I read that I can submit a patch by adding the file to the bug. Just so I understand, I attach the changed css-logic.js file, correct?
Flags: needinfo?(pbrosset)
(In reply to nbeltran14 from comment #5)
> Under Submitting the Patch on the How to submit a patch page
> (https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/
> How_to_Submit_a_Patch), I read that I can submit a patch by adding the file
> to the bug. Just so I understand, I attach the changed css-logic.js file,
> correct?
Not exactly, a patch can indeed be just a file you attach here on this bug (using the attach file link), but this file needs to be specially formatted in order to be recognized as a patch and ultimately applied to the code repository.
Essentially, it needs to be a diff of the code you changed.
This doc should give information about how to do that:
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

For info, this doc also talks about pushing changes to mozreview, this is the now preferred way to submit code changes to the Mozilla project, and is much more similar to how you do things on, say, GitHub. But if you'd rather attach a patch file here, that's fine too, and this doc should have information about that too.
Flags: needinfo?(pbrosset)
Thanks for that. I'm still working on the bug. Some school things got in the way, but I'm still figuring things out. Sorry if this is going slower than expected.
It appears the css-logic.js file has been changed to no longer have the bug that was reported. My understanding of the changes is very limited. It appears that findCssSelector is now imported through loader.lazyImporter and now findCssSelector is assigned to exports.findCssSelector. There is now no tagName. Does this bug now get closed?
Flags: needinfo?(pbrosset)
You are right, the code was apparently just moved in bug 1356415. The code is now in this file: /toolkit/modules/css-selector.js
As far as I can see however, the code still has the problem described in comment 0, so the bug should not be closed.
Flags: needinfo?(pbrosset)
Comment on attachment 8866007 [details]
bug1360132 - 'Escaped tagName with cssEscape in findCssSelector to deal with special chars.'

https://reviewboard.mozilla.org/r/137604/#review141094

Looks good! Thanks for this.
I have 2 comments about the code changes, and then we should be ready to go!

::: toolkit/modules/css-selector.js:90
(Diff revision 1)
>        matches = document.querySelectorAll(selector);
>        if (matches.length === 1) {
>          return selector;
>        }
>        // Maybe it's unique with a tag name?
>        selector = tagName + selector;

I think we also need to escale the tagName here.

::: toolkit/modules/css-selector.js:110
(Diff revision 1)
>    // Not unique enough yet.  As long as it's not a child of the document,
>    // continue recursing up until it is unique enough.
>    if (ele.parentNode !== document) {
>      index = positionInNodeList(ele, ele.parentNode.children) + 1;
>      selector = findCssSelector(ele.parentNode) + " > " +
> -      tagName + ":nth-child(" + index + ")";
> +      CSS.escape(tagName) + ":nth-child(" + index + ")";

You should use `cssEscape` here instead of `CSS.escape`. See line 59 where it's initialized.
Attachment #8866007 - Flags: review?(pbrosset)
Hi Shane,
The code in toolkit/modules/css-selector.js comes from DevTools originally, so I'm doing the review here.
However, do you know if, in time, we need to transfer reviews to someone else? Someone who is either an owner or a peer of the toolkit module?
Flags: needinfo?(mixedpuppy)
Would you like me to wait until you get a response about potentially transferring reviews to someone who works with the toolkit module before I submit my next patch?
Flags: needinfo?(pbrosset)
pbro: can you ramp up mattw? 
nbeltran14: no need to wait to submit, we'll work out review.
Flags: needinfo?(pbrosset)
Flags: needinfo?(mixedpuppy)
Comment on attachment 8866007 [details]
bug1360132 - 'Escaped tagName with cssEscape in findCssSelector to deal with special chars.'

https://reviewboard.mozilla.org/r/137604/#review141490

Looks great, thanks!
I just have one comment (see below) about the commit message. Can you please address it and simply re-push your patch to mozreview? The R+ flag will be preserved.

::: commit-message-b0ff0:1
(Diff revision 2)
> +bug1360132 - 'Escaped tagName on lines 90 and 110 using cssEscape.' r?pbro

Please change the commit message to a sentence that explains what was done and why.
This needs to be a very short summary.

If you feel like you need to add more information, you can always add subsequent lines.

Suggestion:

Bug 1360132 - Escape tagName in findCssSelector to deal with special chars; r=pbro
Attachment #8866007 - Flags: review?(pbrosset) → review+
(In reply to Shane Caraveo (:mixedpuppy) from comment #14)
> pbro: can you ramp up mattw? 
Sure I'd be happy to. The file is rather simple, so that should be easy.
mattw, let me know on IRC when you're online so we can chat about this. My nick is pbro there.
Flags: needinfo?(mwein)
Thanks! I pushed this commit to try server (the CI environment) to make sure mochitests still pass: https://treeherder.mozilla.org/#/jobs?repo=try&revision=d49f491a45bb4285f0246dd420075541232ed513
Ok, let's land this. try results look good.
hg error in cmd: hg push -r tip ssh://hg.mozilla.org/integration/autoland: pushing to ssh://hg.mozilla.org/integration/autoland
searching for changes
remote: adding changesets
remote: adding manifests
remote: adding file changes
remote: added 1 changesets with 1 changes to 1 files
remote: 
remote: 
remote: ************************** ERROR ****************************
remote: Rev 56e78107cfbd needs "Bug N" or "No bug" in the commit message.
remote: nbeltran14 <nbeltran14@mail.wou.edu>
remote: bug1360132 - 'Escaped tagName with cssEscape in findCssSelector to deal with special chars.' r=pbro
remote: 
remote: MozReview-Commit-ID: CadEDXDYeBV
remote: *************************************************************
remote: 
remote: 
remote: transaction abort!
remote: rollback completed
remote: pretxnchangegroup.c_commitmessage hook failed
abort: push failed on remote
I'll try to reland this. The commit message was a bit different than what we normally use, and that made autoland fail.
A space was missing between "bug" and "1360132"
Comment on attachment 8867151 [details]
Bug 1360132 - Escaped tagName with cssEscape in findCssSelector to deal with special chars;

https://reviewboard.mozilla.org/r/138740/#review142040

Carrying R+ over to this patch.
Attachment #8867151 - Flags: review?(pbrosset) → review+
Pushed by pbrosset@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/4e105a4f0ad0
Escaped tagName with cssEscape in findCssSelector to deal with special chars; r=pbro
(In reply to Patrick Brosset <:pbro> from comment #22)
> I'll try to reland this. The commit message was a bit different than what we
> normally use, and that made autoland fail.
> A space was missing between "bug" and "1360132"

Oops. Sorry about that.
https://hg.mozilla.org/mozilla-central/rev/4e105a4f0ad0
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
status-firefox55: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 55
Product: Firefox → DevTools
status-firefox55: fixed → ---
Flags: needinfo?(matthewjwein)
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: