Open
Bug 1221183
Opened 9 years ago
Updated 2 years ago
Double-click on a tag shouldn't only allow you to edit tag but also add whatever you want like classes or id
Categories
(DevTools :: Inspector, enhancement, P3)
Tracking
(Not tracked)
NEW
People
(Reporter: m+mozilla, Unassigned)
Details
Attachments
(2 files, 3 obsolete files)
84.95 KB,
image/gif
|
Details | |
17.22 KB,
patch
|
Details | Diff | Splinter Review |
User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.10; rv:45.0) Gecko/20100101 Firefox/45.0
Build ID: 20151103030248
Steps to reproduce:
- Open devtools
- Double click on a tag like <div>
- Add an attribute like a class: <div class="hello">
Actual results:
It doesn't add hello class.
Expected results:
It should add hello class.
Fun fact, you could also use cmd/ctrl+enter to edit the whole line but it will work or not depending on how many attributes the tag has already.
For instance:
<div class="hello">, you'll be able to ctrl/cmd+enter and add 'id="test"'.
If you have <div>, you won't be able.
If there's already an attribute you can add whatever you want via cmd/ctrl+enter but if there's no attribute, nope.
Comment 2•9 years ago
|
||
I was able to reproduce this bug.
The BrowserConsole shows the following error message and stack trace:
Origin: resource://gre/modules/devtools/server/protocol.js:907
Error: Could not change node's tagName to div id="felafel"
Stack trace:
WalkerActor<.editTagName<@resource://gre/modules/commonjs/toolkit/loader.js -> resource://gre/modules/devtools/server/actors/inspector.js:2693:29
actorProto/</handler@resource://gre/modules/commonjs/toolkit/loader.js -> resource://gre/modules/devtools/server/protocol.js:1013:19
DSC_onPacket@resource://gre/modules/commonjs/toolkit/loader.js -> resource://gre/modules/devtools/server/main.js:1596:15
LocalDebuggerTransport.prototype.send/<@resource://gre/modules/commonjs/toolkit/loader.js -> resource://gre/modules/devtools/transport/transport.js:569:11
makeInfallible/<@resource://gre/modules/commonjs/toolkit/loader.js -> resource://gre/modules/devtools/DevToolsUtils.js:86:14
makeInfallible/<@resource://gre/modules/commonjs/toolkit/loader.js -> resource://gre/modules/devtools/DevToolsUtils.js:86:14
Patrick, any ideas who to complain to about this?
Status: UNCONFIRMED → NEW
Component: Untriaged → Developer Tools: Inspector
Ever confirmed: true
Flags: needinfo?(pbrosset)
Comment 3•9 years ago
|
||
Interesting idea. I think it makes sense to add this feature and it shouldn't be too hard to do.
For now, the text input that appears when you double-click a tagname assumes you're only going to change the tagname, so it fails if you pass an invalid tagname.
At the very least, the markup-view should split at the first non-valid character and use the first part as the tagname.
But, yeah, why not also consider whatever comes after a whitespace to be attributes.
To support this, we'd have to change the code in the onTagEdit function here:
https://dxr.mozilla.org/mozilla-central/source/devtools/client/markupview/markup-view.js#2954
we could split/sanitize the string and, on one hand, do the tag edit, and on the other hand, create attributes by calling _applyAttributes.
I'm puzzled by comment 1 though, for me, ctrl+enter doesn't do anything special that enter doesn't do already, and that's just turning the tagname to edit mode (same as when you double-click).
Mentor: pbrosset
Flags: needinfo?(pbrosset)
Comment 4•9 years ago
|
||
With another one _applyAttributes added we have 3 invocations within markup-view.js. And they have much in common - undo / redo part. I put it into applyAttributes and second parameter now is used not only for positioning new attributes, but for removing edited node too.
I found 3 files where tests can be placed:
* browser_markupview_tag_edit_03.js
* browser_markupview_tag_edit_10.js
* browser_markupview_tag_edit_11.js
All these files have tests which are related to tags. I picked browser_markupview_tag_edit_03.js and renamed it to browser_markupview_tag_edit_03-tag.js. This way it is more obvious in what file tests for tags should be placed.
Tests reproduce two cases: adding attributes without tag change and with tag change. In second case I found that undo doesn't work. Can I file a bug about that?
Tests are passed, but I see errors:
> console.error:
> Message: Error: Connection closed, pending request to server1.conn0.domwalker28, type children failed
I haven't found what causes these error to occur yet.
Attachment #8685116 -
Flags: feedback?(pbrosset)
Comment 5•9 years ago
|
||
And I have a question about JSDoc. Finally I setup ESLint and haven't found rules for formatting.
In markup-view.js I see different styles, but I'm trying to stick to this one:
> /**
> * Description.
> *
> * @param {type} variable
> * Description.
> * @param {type} variable
> * Description.
> * @return {type}
> * Description.
> */
Is it accepted style in DevTools team? Can I reformat existing JSDoc to this style while editing?
Flags: needinfo?(pbrosset)
Comment 6•9 years ago
|
||
(In reply to Grisha Pushkov from comment #5)
> And I have a question about JSDoc. Finally I setup ESLint and haven't found
> rules for formatting.
> In markup-view.js I see different styles, but I'm trying to stick to this
> one:
>
> > /**
> > * Description.
> > *
> > * @param {type} variable
> > * Description.
> > * @param {type} variable
> > * Description.
> > * @return {type}
> > * Description.
> > */
>
> Is it accepted style in DevTools team? Can I reformat existing JSDoc to this
> style while editing?
I originally tried to enable the jsdoc rule in ESLint (valid-jsdoc), but it had a bug that prevented us from using it (https://github.com/eslint/eslint/issues/2270), so for now, it's disabled.
The guideline is to try and stick to the jsdoc style that's in the file you're editing currently. So that, though we're not consistent throughout the codebase, we at least are consistent within a file.
The format you pasted above looks fine to me. I think that's the most commonly used in devtools.
If you feel like reformatting all jsdoc comments in a file so they are more consistent, then that's fine, but please do it in a separate commit so the file history is easier to navigate.
Flags: needinfo?(pbrosset)
Comment 7•9 years ago
|
||
> we at least are consistent within a file
Ok, will keep it in mind. I'm not going to reformat every JSDoc in file, only those which I'm editing.
Comment 8•9 years ago
|
||
About errors in my comment 4. I was able to reproduce it without applied patch and with modified browser_markupview_tag_edit_03.js (tests below setEditableFieldValue were removed).
browser_markupview_tag_edit_03.js: https://pastebin.mozilla.org/8851927
output: https://pastebin.mozilla.org/8851928
I already found that in my patch yield on "markupmutation" is missing, but errors still persist if I add it.
Seems that some async operations aren't finished when all tests passed, so we get these errors.
Flags: needinfo?(pbrosset)
Comment 9•9 years ago
|
||
(In reply to Grisha Pushkov from comment #8)
> About errors in my comment 4. I was able to reproduce it without applied
> patch and with modified browser_markupview_tag_edit_03.js (tests below
> setEditableFieldValue were removed).
>
> browser_markupview_tag_edit_03.js: https://pastebin.mozilla.org/8851927
> output: https://pastebin.mozilla.org/8851928
>
> I already found that in my patch yield on "markupmutation" is missing, but
> errors still persist if I add it.
> Seems that some async operations aren't finished when all tests passed, so
> we get these errors.
That's right, there are other async requests sent from the markup-view to the server after the markupmutation event has fired. And the test failed because it sees pending requests (in this case, the 'children' request which is what the markup-view does to know the children of an element).
What's weird is that we have other tests in the same directory that seem to do the same thing and are not failing.
I think on top of waiting for markupmutation, you need to also wait for inspector-updated.
Flags: needinfo?(pbrosset)
Comment 10•9 years ago
|
||
Comment on attachment 8685116 [details] [diff] [review]
1221183-attributes-in-tag.patch
Review of attachment 8685116 [details] [diff] [review]:
-----------------------------------------------------------------
This looks great.
A couple of questions though:
- Aren't you already adding the test-actor machinery in head.js in another bug? I've reviewed a change from you that did this but can't remember which bug this was. Instead of doing it here too, you should just depend on that other bug and build this patch on top of it.
- I haven't tried this but what's our story going to be like for undo in the case where you change both the tagName and attributes? I see that your test does not exercise this. What happens today if you try to undo? Ideally it should undo both the tagName and attribute changes, but that's ok to do as a follow-up bug if needed.
Attachment #8685116 -
Flags: feedback?(pbrosset) → feedback+
Comment 11•9 years ago
|
||
(In reply to Grisha Pushkov from comment #4)
> Tests reproduce two cases: adding attributes without tag change and with tag
> change. In second case I found that undo doesn't work. Can I file a bug
> about that?
Oh, sorry, I didn't see this comment when I wrote comment 10. So, yes, please let's investigate what does and does not work for undo in this case, and file a bug to fix what doesn't work.
Comment 12•9 years ago
|
||
After adding yields on "markupmutation" (as before, I believe) errors disappeared. Maybe I missed something last time.
> Aren't you already adding the test-actor machinery in head.js in another bug? I've reviewed a change
> from you that did this but can't remember which bug this was. Instead of doing it here too, you should > just depend on that other bug and build this patch on top of it.
Yes, I did. Don't know in which order bugs could be merged with fx-team, so I prefer to not block or be blocked on others. Later I can rebase and upload patch again. Will it be ok?
Will create bug for undo / redo on tag change soon.
Flags: needinfo?(pbrosset)
Comment 13•9 years ago
|
||
(In reply to Grisha Pushkov from comment #12)
> > Aren't you already adding the test-actor machinery in head.js in another bug? I've reviewed a change
> > from you that did this but can't remember which bug this was. Instead of doing it here too, you should > just depend on that other bug and build this patch on top of it.
> Yes, I did. Don't know in which order bugs could be merged with fx-team, so
> I prefer to not block or be blocked on others. Later I can rebase and upload
> patch again. Will it be ok?
Sure, you can do it anyway you want, as long as the right commits go in the right order at the end.
Flags: needinfo?(pbrosset)
Comment 14•9 years ago
|
||
> Will create bug for undo / redo on tag change soon.
Here it is:
https://bugzilla.mozilla.org/show_bug.cgi?id=1226021
Comment 15•9 years ago
|
||
In this patch I added reviewer to commit message and yields on "markupmutation".
> Sure, you can do it anyway you want, as long as the right commits go in the
> right order at the end.
Want to clarify to be sure. One of these patches with duplicated code in test-actor.js can go without being blocked, and after first one is commited I should rebase and remove duplicated code from others.
For example, if this patch is going to fx-team, I will rebase other patches on it. And there is no order in advance, patch is commited as far as it is ready.
Disadvantage of duplicated code besides rebasing is that reviewer looks at the same code many times. Changes in test-actor.js isn't big enough, hope there are no inconveniences here.
Attachment #8685116 -
Attachment is obsolete: true
Attachment #8689235 -
Flags: review?(pbrosset)
Comment 16•9 years ago
|
||
(In reply to Grisha Pushkov from comment #15)
> Want to clarify to be sure. One of these patches with duplicated code in
> test-actor.js can go without being blocked, and after first one is commited
> I should rebase and remove duplicated code from others.
Yes.
> For example, if this patch is going to fx-team, I will rebase other patches
> on it. And there is no order in advance, patch is commited as far as it is
> ready.
>
> Disadvantage of duplicated code besides rebasing is that reviewer looks at
> the same code many times. Changes in test-actor.js isn't big enough, hope
> there are no inconveniences here.
Correct, those are the 2 disadvantages indeed (more work for you and reviewer looks at the code twice). But you're right, in this case, the change is small enough that it's probably easier for you.
In general, people tend to work on a "series" of patches (where order matters) instead of working in "parallel" (where duplication is sometimes needed), as this can avoid confusion. But again, this is a simple enough change that it's really up to you. I think it can sometimes help telling the reviewer in advance though, so they don't re-review a piece of duplicated code they've already reviewed.
On other approach: create a third bug and work on the common part in there first. If it's simple enough, there are good changes it will be reviewed and land quickly.
Comment 17•9 years ago
|
||
Comment on attachment 8689235 [details] [diff] [review]
1221183-attributes-in-tag.patch
Review of attachment 8689235 [details] [diff] [review]:
-----------------------------------------------------------------
One thing I noticed while playing with this patch:
If you modify attributes but not the tagName and then try to undo, everything's fine.
If you do change the tagName though, and undo, then it doesn't work, but that's expected. However, what's not expected is that this error is shown in the browser console:
Handler function LocalDebuggerTransport instance's this.other.hooks.onPacket threw an exception: TypeError: to is null
Stack: DSC_onPacket@resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/server/main.js:1617:11
LocalDebuggerTransport.prototype.send/<@resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/shared/transport/transport.js:569:11
makeInfallible/<@resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/shared/DevToolsUtils.js:87:14
makeInfallible/<@resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/shared/DevToolsUtils.js:87:14
Line: 1617, column: 11
Also note that I have filed bug 1227059 for a possibly related bug (which I reproduced on about:home only so far).
I only have 3 comments, so R+ for this.
Please push to try (you can use http://trychooser.pub.build.mozilla.org/ to create a try syntax (also there's some doc here: https://wiki.mozilla.org/ReleaseEngineering/TryServer).
I usually use a syntax like this: http://trychooser.pub.build.mozilla.org/
::: devtools/client/markupview/markup-view.js
@@ +2958,5 @@
> + // Committed value can contain attributes.
> + // Consider tag to be before first whitespace and remaining to be attributes.
> + let newTagName = value.match(/^[^ ]*/)[0];
> + let attributes = value.slice(newTagName.length);
> + this._applyAttributes(attributes, null);
I think you should wrap this with
if (attributes.length) { ... }
to avoid doing anything if there was no attributes changed.
::: devtools/client/markupview/test/browser_markupview_tag_edit_03-tag.js
@@ +67,5 @@
> +
> + let parentInfo = yield getNodeInfo(selector);
> + is(parentInfo.tagName.toLowerCase(), "p", "tagname wasn't changed");
> +
> + yield undoChange(inspector);
Can you add another attributes assertion after the undo to make sure the new attributes are gone?
@@ +84,5 @@
> + ok(isEqualAttributes((yield testActor.getAttributes(selector)),
> + ["id", "b", "c"]), "attributes were added");
> +
> + let parentInfo = yield getNodeInfo(selector);
> + is(parentInfo.tagName.toLowerCase(), "div", "tagname was changed");
Can you add a comment at the end of this function like:
// XXX: After bug 1226021 is fixed, this test function should also
// test undoing the tag and attribute change here.
Attachment #8689235 -
Flags: review?(pbrosset) → review+
Comment 18•9 years ago
|
||
> If you do change the tagName though, and undo, then it doesn't work, but that's expected.
> However, what's not expected is that this error is shown in the browser console...
It was because attributes were applied before new node creation, so undo was done for already deleted node. I looked at bug 1227059, seems to be different problem. Also I can reproduce error with editing second div too.
Now newly created node's container are passed as parameter to "reselectedonremoved" event.
And in onTagEdit two _applyAttributes invocations came out: one for unchanged tag, another for changed.
> I think you should wrap this with
> if (attributes.length) { ... }
> to avoid doing anything if there was no attributes changed.
Added this check to _applyAttributes.
> Can you add another attributes assertion after the undo to make sure the new attributes are gone?
Done.
> Can you add a comment at the end of this function like...
Really like these reminders. Already helped me twice.
I spotted usage of todo() which I put there.
Going to push to try.
ni you to take a look at these changes.
Attachment #8689235 -
Attachment is obsolete: true
Flags: needinfo?(pbrosset)
Attachment #8692104 -
Flags: review+
Comment 19•9 years ago
|
||
My last patch breaks client/markupview/test/browser_markupview_tag_edit_01.js test. Committed value can be empty when editing attribute.
Problem in this condition:
>+ _applyAttributes: function(value, editedAttribute) {
> + if (!value) {
> + return;
> + }
I figured out these possible cases for _applyAttibutes:
1. Value is empty (hence attributes are empty) and we are editing attribute. Continue in _applyAttributes.
2. Attributes are empty and not case (1). Value is empty or isn't parsable. Return.
3. Value is parsable and we got attributes. It's ok, continue in _applyAttributes.
I got a following condition:
> // Empty value is valid only when editing attribute.
> // In all other cases empty attributes aren't applied.
> if (attrs.length == 0 && !(value.length == 0 && editedAttribute)) {
> return;
> }
And failing client/markupview/test/browser_markupview_tag_edit_12.js test on "Entering an invalid attribute to delete the attribute" :(
Here are reasons why "invalid attribute to delete the attribute" isn't good:
1. Changing tag with invalid name restores original tag. We should do the same for attributes, or remove tag to follow attributes behavior.
2. Applying this behavior to newAttr (and attributes in tag after this patch) we get this example:
> <div class="original">
> change class:
> <div class="changed">
> add invalid or empty value to newAttr:
> <div class="change" [123]>
> get this markup:
> <div class="change">
> try to undo and markup is unchanged:
> <div class="change">
> undo again:
> <div class="original">
So committed empty or invalid value to newAttr is saved as action in doMods / undoMods.
Or we can apply any value when editing attribute, and not empty attributes for tag / newAttr change?
For now I have changed invalid value to empty string in test.
Attachment #8692104 -
Attachment is obsolete: true
Comment 20•9 years ago
|
||
Comment 21•9 years ago
|
||
And two more notes:
1. If we change attribute, rename tag and then undo - there is an error as was before with attributes and tag change at the same time and then undo. I will add it to bug 1226021. Somehow we should change node after creating actions in doMods / undoMods.
2. Should we destroy listener when editTagName failed and "reselectedonremoved" wasn't fired?
> // Apply attributes after new node is created.
> this.markup.once("reselectedonremoved", (e, container) => {
> container.editor._applyAttributes(attributes, null);
> });
Comment 22•9 years ago
|
||
Comment 23•9 years ago
|
||
Comment on attachment 8692202 [details] [diff] [review]
1221183-attributes-in-tag.patch
Review of attachment 8692202 [details] [diff] [review]:
-----------------------------------------------------------------
::: devtools/client/markupview/markup-view.js
@@ +2967,5 @@
> }
>
> + // Committed value can contain attributes.
> + // Consider tag to be before first whitespace and remaining - attributes.
> + let newTagName = value.match(/^[^ ]*/)[0];
I think we should use this regex instead: /^\S+/)
That way we'd catch all non-whitespace characters.
@@ +2973,5 @@
> +
> + // Apply attributes even if tag isn't changed.
> + if (newTagName.toLowerCase() === this.node.tagName.toLowerCase() ||
> + !("editTagName" in this.markup.walker)) {
> + this._applyAttributes(attributes, null);
Wait, I'm confused, shouldn't we just always apply attributes (as long as attributes is a non-empty string).
If the tagname has changed, and we're targeting a backend that has the editTagName method, then we won't change the attributes.
You should take this out of this if block, move it before and wrap it in an if (attributes) condition.
Updated•9 years ago
|
Flags: needinfo?(pbrosset)
Comment 24•9 years ago
|
||
> Wait, I'm confused, shouldn't we just always apply attributes (as long as attributes is a non-empty string).
In previous patch attributes were applied at the top of onTagEdit. But it caused a problem when attributes are applied -> tag name changed -> new node created -> trying undo attributes -> getting error that node isn't exists for reverting attributes.
It's related to another problem which I described in comment 21, "If we change attribute, rename tag and then undo - there is an error...".
Maybe we should add attributes to current done (without duplication which was added), and fix undo / redo errors in bug 1226021?
Flags: needinfo?(pbrosset)
Comment 25•9 years ago
|
||
Wow, so sorry for the delay here, this needinfo somehow completely slipped under my radar. My review request queue is always pretty full and I address these first, so I didn't see this one for a long while.
(In reply to Grisha Pushkov from comment #24)
> In previous patch attributes were applied at the top of onTagEdit. But it
> caused a problem when attributes are applied -> tag name changed -> new node
> created -> trying undo attributes -> getting error that node isn't exists
> for reverting attributes.
>
> It's related to another problem which I described in comment 21, "If we
> change attribute, rename tag and then undo - there is an error...".
> Maybe we should add attributes to current done (without duplication which
> was added), and fix undo / redo errors in bug 1226021?
I agree that whatever undo/redo problems occur when you change both a tagname and its attributes should be fixed in bug 1226021. Maybe we should fix that one first in fact.
Knowing the limitations related to how we deal with undo/redo right now, it seems like it would make sense to first try to rename the tag, wait for that to happen, and then change the attributes if needed. At least, the attribute changes would be undoable this way.
Flags: needinfo?(pbrosset)
Updated•9 years ago
|
Severity: normal → enhancement
Updated•6 years ago
|
Product: Firefox → DevTools
Updated•4 years ago
|
Mentor: patrickbrosset+bugzilla
Updated•2 years ago
|
Severity: normal → S3
You need to log in
before you can comment on or make changes to this bug.
Description
•