1248274 - Clicking property values placed right after filter/color circle fails if I was editing corresponding property name

Status: VERIFIED FIXED

(DevTools :: Inspector: Rules, defect, P2)

Description

[arni2033]

>>>   My Info:   Win7_64, Nightly 47, 32bit, ID 20160210071115
STR:
1. Open the following "data:" url or click URL in the form above
>   data:text/html,<style>body{filter: grayscale(100%) contrast(17%) drop-shadow(0px 2px 4px yellow);background:none repeat scroll 0% 0% transparent}
2. Open devtools -> Inspector, open "Rules" tab in sidebar
3.A) Click "filter" string in sidebar. Then click string "grayscale"
3.B) Click "filter" string in sidebar. Then click string "yellow"
3.C) Click "color"  string in sidebar. Then click string "transparent"

AR:  Textarea opened for editing property name collapses. That's all.
ER:  In addition, a new textarea for editing property value should appear, because (!)
     it always happens when I click normal strings in property value (e.g. "none", "contrast")

Comment 1

[arni2033]

That was a typo.    "autoscroll circle" -> "filter/color circle"

Comment 2

[Nicolas Chevobbe [:nchevobbe]]

Here's my analysis of the problem :

When the name is in edit mode, `mousedown` on the value element fire the `blur` event on the inplace editor, which lead to a call to the `update` function  on the TextPropertyEditor.
In the update function, the valueSpan element is cleared ( https://dxr.mozilla.org/mozilla-central/source/devtools/client/inspector/rules/views/text-property-editor.js#360 ), and then re-populated.

This is the cause of the issue for "swatch" values ( color, filter, ...).
For such values, there is an extra element which contains the swatch and the corresponding value.
So when the property name is edited, if the user click this extra element, the input is blurred, which lead to the valueSpan to be cleared, and thus to this extraElement to be destroyed.
Because it is destroyed on mousedown, the following mouseup event ( and the resulting click event ), can't be bubbled up to the valueSpan.

This clearing can lead to another problem : if you edit the name, and then click on the swatch, the color tooltip is shown, but not at the right place ( I guess because the original element is destroyed ).

One quick fix might be to create the editableField on mouseup instead of click ( which can be done with options.trigger when calling editableField (https://dxr.mozilla.org/mozilla-central/source/devtools/client/shared/inplace-editor.js#145), but that leads to other problems with the tooltip.

Comment 3

[Nicolas Chevobbe [:nchevobbe]]

Attachment: rule_click.patch

This is kind of hacky, but as discussed earlier this week on IRC, there shouldn't be a clean way to deal with this. 

So what we do here is to detect a pending click when mousedown on the valueSpan.
And when update() is called, if we have a pending click, trigger that click. 

The reason I'm asking for feedback is that I also handled click on the swatch. For now, when we edit the property name name, and click on the swatch, the tooltip is displayed in the upper right corner of the devtools window, and it isn't actually linked with the clicked swatch (changing the color won't update the rule), because the swatch element is being re-created after the tooltip is shown.

We could not handle it at all, and enter edit mode on the value when clicking the swatch, but I find it a disturbing : if I click the swatch, I'm expecting the tooltip to show up.

Even if it's working well, I am also unsure about how I handle this. When mousedown on the valueSpan, if the targeted element is a swatch, I get his "swatch index" in the valueSpan. And then, in the update function, I retrieve the swatch if the same index and call click() on it.

Comment 4

[Patrick Brosset <:pbro>]

(In reply to Nicolas Chevobbe [:nchevobbe] from comment #3)
> Created attachment 8749597 [details] [diff] [review]
> rule_click.patch
> 
> This is kind of hacky, but as discussed earlier this week on IRC, there
> shouldn't be a clean way to deal with this. 
First of all, thanks for working on this. I've tested your patch locally and it seems to work fine! As you said, there might not be a nice way of fixing this. I'll take a look at the code changes now.
> So what we do here is to detect a pending click when mousedown on the
> valueSpan.
> And when update() is called, if we have a pending click, trigger that click. 
This sounds good to me.
> The reason I'm asking for feedback is that I also handled click on the
> swatch. For now, when we edit the property name name, and click on the
> swatch, the tooltip is displayed in the upper right corner of the devtools
> window, and it isn't actually linked with the clicked swatch (changing the
> color won't update the rule), because the swatch element is being re-created
> after the tooltip is shown.
Good catch on this one. I wonder if arni2033 didn't file a separate bug for this one.
There's one use case we need to keep in mind though, if I change the property name to something that doesn't accept colors (like "test"), then the color swatch won't be re-created, and in that case, we shouldn't try to show any tooltip at all. This seems to work fine with your patch too. I'll take a look at the code.

Comment 5

[Patrick Brosset <:pbro>]

Comment on attachment 8749597 [details] [diff] [review]
rule_click.patch

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

Well, this works well, and doesn't look too bad!
Update is async, so I don't see any other way, given our current code, than to remember that we have clicked, and then re-trigger that click when the update is done.
We'll need tests for this, which will need to wait for the right events to know when the value has been updated.

::: devtools/client/inspector/rules/views/text-property-editor.js
@@ +23,5 @@
>  
>  const HTML_NS = "http://www.w3.org/1999/xhtml";
>  const IOService = Cc["@mozilla.org/network/io-service;1"]
>                    .getService(Ci.nsIIOService);
> +const sharedSwatchClass = "ruleview-swatch";

nit: please use UPPER_CASE_WITH_UNDERSCORE notation for constants.

@@ +216,5 @@
>            this.valueSpan.click();
>          }
>        }, false);
>  
> +      this.valueSpan.addEventListener("mousedown", (event) => {

Above this line you'll need to add some comments explaining what we are doing here.
That we're waiting for the update to happen on blur, and so we're remembering where the user has clicked on mousedown.
And that we only do this for sub-elements, if it's the valueSpan itself, we don't care and why, etc...

@@ +224,5 @@
> +        this._hasPendingClick = true;
> +        if (event.target.classList.contains(sharedSwatchClass)) {
> +          let swatches = this.valueSpan.querySelectorAll(
> +            `.${sharedSwatchClass}`);
> +          let index = Array.from(swatches).indexOf(event.target);

Instead of Array.from:

[...this.valueSpan.querySelectorAll(`.${sharedSwatchClass}`)]

@@ +434,5 @@
>          angleSpan.on("unit-change", this._onSwatchCommit);
>        }
>      }
>  
> +    if (this._hasPendingClick) {

Also above this line, please add a comment explaining that now that we've updated the value, we might have a pending click and that if we do, we need to trigger it, on the right element.

::: devtools/client/shared/output-parser.js
@@ +457,5 @@
>        return;
>      }
>  
> +    // Prevent click event to be fired to not show the tooltip
> +    event.stopPropagation();

I don't understand this change (and the one below in _onAngleSwatchMouseDown). Can you explain to me the problem you are solving here?

Comment 6

[Nicolas Chevobbe [:nchevobbe]]

Thanks for your feedback, I'll address your comments and will set up some tests. 

> I don't understand this change (and the one below in
> _onAngleSwatchMouseDown). Can you explain to me the problem you are solving
> here?

Sure. So those function handle unit switching for color and angle swatches.
It surely is an edge case but if you edit the property name, and then, hold shift and click the swatch, if we don't make those changes, my patch will detect that there were a mousedown on a swatch, and will try to re-trigger it after the update. 
So for the color, it would trigger a color tooltip opening after the update call, which is unwanted.

We could handle this case too in text-property, but this would get quite messy (e.g. we could check if switch key is pressed before raising hasPendingClick, but AFAIK we can open bezier-curve tooltip with shift-click, so we should add some extra tests, ...) . 

So with this change, if we do shift + click, we do not bubble up the mousedown event, unit change is made, update is called, and everything works fine.

Comment 7

[arni2033]

(In reply to Patrick Brosset <:pbro> from comment #4)
> > For now, when we edit the property name name, and click on the swatch, the tooltip is displayed
> > in the upper right corner of the devtools window, and it isn't actually linked with the clicked
> > swatch (changing the color won't update the rule)
> Good catch on this one. I wonder if arni2033 didn't file a separate bug for this one.
Well, I did file this obvious thing, and added it in "See also" list -> bug 1248275. 
I did so even though it may be caused by the same underlying issue.

Comment 8

[Nicolas Chevobbe [:nchevobbe]]

> Well, I did file this obvious thing, and added it in "See also" list -> bug
> 1248275. 
> I did so even though it may be caused by the same underlying issue.

Thanks arni2033 for chiming in. Bug 1248275 will be resolved by my patch. I'm gonna assign it to me so no one will work on it for nothing, and I'll mark it as RESOLVED when this one lands. Is it okay for you ?

Comment 9

[Nicolas Chevobbe [:nchevobbe]]

Attachment: Bug 1248274 - Fix click on augmented value elements (swatch, url,…) on the RuleView when property's name is being edited.

Add a test to ensure the bug is fixed

Review commit: https://reviewboard.mozilla.org/r/52632/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/52632/

Comment 10

[Nicolas Chevobbe [:nchevobbe]]

TRY job for my patch was okay : https://treeherder.mozilla.org/#/jobs?repo=try&revision=eff6687a5735

Comment 11

[Nicolas Chevobbe [:nchevobbe]]

Comment on attachment 8752560 [details]
Bug 1248274 - Fix click on augmented value elements (swatch, url,…) on the RuleView when property's name is being edited.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/52632/diff/1-2/

Comment 12

[Patrick Brosset <:pbro>]

Comment on attachment 8752560 [details]
Bug 1248274 - Fix click on augmented value elements (swatch, url,…) on the RuleView when property's name is being edited.

https://reviewboard.mozilla.org/r/52632/#review49940

Almost there. A few changes to then test, and we missed something with URLs. See below.

::: devtools/client/inspector/rules/test/browser_rules_edit-property_10.js:34
(Diff revision 2)
> +
> +  info("Test click on color span while editing property name");
> +  yield selectNode("#testid", inspector);
> +  let ruleEditor = getRuleViewRuleEditor(view, 1);
> +
> +  // Get "color" rule editor

Please replace this comment with an `info` instead:

`info("Focus the color name space");`

It helps with debugging.

::: devtools/client/inspector/rules/test/browser_rules_edit-property_10.js:38
(Diff revision 2)
> +  let onUpdate = new Promise(resolve => {
> +    let observer = new MutationObserver((mutation) => {
> +      observer.disconnect();
> +      resolve();
> +    });
> +    observer.observe(propEditor.valueSpan, {childList: true});
> +  });

A few comments about this:

- You need it again later (line 62), so please move this to a function you can reuse.
- This requires a comment in the code that explains what this is doing and why you need it.
- ruleview-changed isn't emitted in this case so you can't use it, but it seems to me that perhaps we should have an event that the rule-view fires when a value has been re-constructed. Which makes me think, in this case the name hasn't changed, should we even be updating the value? TextPropertyEditor.update should do nothing in this case, right? Of course we should still update if the name has changed, because what used to be a color before might now be just a string.

::: devtools/client/inspector/rules/test/browser_rules_edit-property_10.js:93
(Diff revision 2)
> +
> +  let spectrum = yield colorPicker.spectrum;
> +  is(spectrum.rgb, "200,170,140,0.5", "The correct color picker was shown");
> +
> +  // Wait for the ruleview-changed event to avoid pending requests.
> +  yield onRuleViewChanged;

This feels a bit lost here, when I read it first, I wasn't really sure what was triggering this event.
It turns out that it is emitted when you focus the background name, because doing so blurs the previous color value. And blurring a value always emits the ruleview-changed event.

So I suggest being more explicit about this:
at the end of the first test (after you've changed that the value span is in edit mode (with an inplaceditor)), then listen to the event there, with a comment saying that burring will emit the event. Then blur the field, and wait for the event.
This way you can start the second part of the test without having to worry about this.

Also, related, I would move the 2 parts into generator functions that the main part in add_task can then call. Makes it easier to read the overflow flow the test.

And also, please add a third part that checks that the right thing happens when you shift+click.

::: devtools/client/inspector/rules/views/text-property-editor.js:230
(Diff revision 2)
> +      this.valueSpan.addEventListener("mousedown", (event) => {
> +        if (event.target === this.valueSpan) {
> +          return;
> +        }
> +        this._hasPendingClick = true;
> +        if (event.target.classList.contains(SHARED_SWATCH_CLASS)) {

I just realized one thing: swatches are not the only type of children elements that a valueSpan can have. There are URLs too.
For example: `background: url(path/to/image.gif);` will create a child element for the URL itself, and clicking it while the name is in edit mode won't work.

Comment 13

[Nicolas Chevobbe [:nchevobbe]]

Comment on attachment 8752560 [details]
Bug 1248274 - Fix click on augmented value elements (swatch, url,…) on the RuleView when property's name is being edited.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/52632/diff/2-3/

Comment 14

[Patrick Brosset <:pbro>]

Comment on attachment 8752560 [details]
Bug 1248274 - Fix click on augmented value elements (swatch, url,…) on the RuleView when property's name is being edited.

https://reviewboard.mozilla.org/r/52632/#review52174

::: devtools/client/inspector/rules/test/browser_rules_edit-property_10.js:42
(Diff revision 3)
> +function* testColorValueSpanClick(ruleEditor, view) {
> +  info("Test click on color span while editing property name");
> +  let propEditor = ruleEditor.rule.textProps[0].editor;
> +  let colorSpan = propEditor.valueSpan.querySelector(".ruleview-color");
> +
> +  info("Focus the color name space");

s/space/span

::: devtools/client/inspector/rules/test/browser_rules_edit-property_10.js:75
(Diff revision 3)
> +  info("Test shift + click on color swatch while editing property name");
> +  let propEditor = ruleEditor.rule.textProps[1].editor;
> +  let swatchSpan = propEditor.valueSpan.querySelectorAll(
> +    ".ruleview-colorswatch")[2];
> +
> +  info("Focus the background name space");

s/space/span

::: devtools/client/inspector/rules/views/text-property-editor.js:853
(Diff revision 3)
> +  /**
> +   * Returns a CSS selector that can be used to retrieve a child element
> +   * from one of its parent node
> +   * to get the child element.
> +   *
> +   * @param {Node} parentEl
> +   *        The root node from where we want to get the child element.
> +   * @param {Node} childEl
> +   *        The childEl node we want the selector for.
> +   * @param {String} childSelector
> +   *        An existing direct child selector to append to the one which
> +   *        will be computed.
> +   *
> +   * @return {String} the CSS selector.
> +   */

This jsdoc comment is indented, it should not.

::: devtools/client/inspector/rules/views/text-property-editor.js:868
(Diff revision 3)
> +   *        An existing direct child selector to append to the one which
> +   *        will be computed.
> +   *
> +   * @return {String} the CSS selector.
> +   */
> +function getChildSelectorFromParent(parentEl, childEl, childSelector = "") {

I've spent a long time on this function, trying to find a way to maybe avoid having to do this.
It's not that I don't like the code, it looks ok and does the job, but it feels bad having to resort to scanning the DOM in order to remember where to click once an update was made.

In fact, you could even imagine tricking the thing a little bit.
Say you have `color:red`, then edit `color` and change it to `filter`, and then click on the color swatch next to `red`. This will end up getting the selector for this color swatch, and then doing a click on the element that is at the same location, but when the property has been refreshed, the thing that is at that location isn't a color swatch anymore, it's a cssfilter swatch.
This is an edge case, and I don't think it's a problem anyway, in fact it's nice that it does this. But it's just to highlight the fact that we don't control much here, we just scan the DOM before a refresh, not knowing what's in there, and  then after update try and get an element that is at the same place.
(btw, if the property name has changed, I think we could almost just cancel the pending click, when the name changes, the meaning of the value changes too, and so there are a lot of chances that it won't have swatches anymore, or have them at different locations).

So, on one hand, this is very generic and therefore simple and short, but on the other hand it's messy that we don't have a proper model for the data shown in the rule-view and so we have to resort to reading the DOM to know the state of things.

Another option would be to have something not generic here, but very specific to the rule-view. Detect which element the mousedown occurred on, and store some data about it like: first color swatch, or second url, etc...
If we just bail out when the name changes, then we can assume that the same swatches and urls are going to be re-rendered when the field if blurred, and so just storing the type of element that was clicked on and its index (or whatever) would work just fine, right?

Happy to be convinced otherwise, let me know what you think. If we end up agreeing that your solution is best then I'll R+ this.

::: devtools/client/inspector/rules/views/text-property-editor.js:884
(Diff revision 3)
> +
> +  if (childEl.parentNode !== parentEl) {
> +    selector = getChildSelectorFromParent(
> +      parentEl, childEl.parentNode, selector);
> +  } else {
> +    selector = `:scope > ${selector}`;

I was a bit surprised by `:scope` at first, I'm not used in seeing it, but it makes sense since you're using only nth-child.

Comment 15

[Nicolas Chevobbe [:nchevobbe]]

https://reviewboard.mozilla.org/r/52632/#review52174

> This jsdoc comment is indented, it should not.

oh, I reproduced what he have in this file (see `_previewValue`, `getValueAndExtraProperties`, ...) in order to be consistent. Should I change this one anyway ?

> I've spent a long time on this function, trying to find a way to maybe avoid having to do this.
> It's not that I don't like the code, it looks ok and does the job, but it feels bad having to resort to scanning the DOM in order to remember where to click once an update was made.
> 
> In fact, you could even imagine tricking the thing a little bit.
> Say you have `color:red`, then edit `color` and change it to `filter`, and then click on the color swatch next to `red`. This will end up getting the selector for this color swatch, and then doing a click on the element that is at the same location, but when the property has been refreshed, the thing that is at that location isn't a color swatch anymore, it's a cssfilter swatch.
> This is an edge case, and I don't think it's a problem anyway, in fact it's nice that it does this. But it's just to highlight the fact that we don't control much here, we just scan the DOM before a refresh, not knowing what's in there, and  then after update try and get an element that is at the same place.
> (btw, if the property name has changed, I think we could almost just cancel the pending click, when the name changes, the meaning of the value changes too, and so there are a lot of chances that it won't have swatches anymore, or have them at different locations).
> 
> So, on one hand, this is very generic and therefore simple and short, but on the other hand it's messy that we don't have a proper model for the data shown in the rule-view and so we have to resort to reading the DOM to know the state of things.
> 
> Another option would be to have something not generic here, but very specific to the rule-view. Detect which element the mousedown occurred on, and store some data about it like: first color swatch, or second url, etc...
> If we just bail out when the name changes, then we can assume that the same swatches and urls are going to be re-rendered when the field if blurred, and so just storing the type of element that was clicked on and its index (or whatever) would work just fine, right?
> 
> Happy to be convinced otherwise, let me know what you think. If we end up agreeing that your solution is best then I'll R+ this.

> Say you have color:red, then edit color and change it to filter, and then click on the color swatch next to red. This will end up getting the selector for this color swatch, and then doing a click on the element that is at the same location, but when the property has been refreshed, the thing that is at that location isn't a color swatch anymore, it's a cssfilter swatch.

Good point

> btw, if the property name has changed, I think we could almost just cancel the pending click, when the name changes, the meaning of the value changes too, and so there are a lot of chances that it won't have swatches anymore, or have them at different locations

I don't know if we should do this. I know this is an edge case, but the user could changed to color to background-color (or border-color to border-top-color), and immediatly want to change the color value. 

> Another option would be to have something not generic here, but very specific to the rule-view. Detect which element the mousedown occurred on, and store some data about it like: first color swatch, or second url, etc...

So yes, It would looks like what we had in the first patch, plus URL. I hadn't did it that way specifically because I wanted something more generic, to avoid having to pile-up each of the clickable elements we could have in a property value. For example, something I have in mind is having an augmented element for CSS variables (maybe a link on it, or an "inspect" icon that would display the rule where it is defined, or something else ...), and with the non-generic solution, we should be careful to handle it in order to not have the same kind of bug. 

I do not have strong feeling for this, and I do think as well that building this selector is not ideal. Maybe we don't introduce new property's value elements that much and that a simple list would be fine.
You are more used to work on this and have a broader vision, so I'll implement whatever seems better for you

Comment 16

[Patrick Brosset <:pbro>]

(In reply to Nicolas Chevobbe [:nchevobbe] from comment #15)
> https://reviewboard.mozilla.org/r/52632/#review52174
> 
> > This jsdoc comment is indented, it should not.
> 
> oh, I reproduced what he have in this file (see `_previewValue`,
> `getValueAndExtraProperties`, ...) in order to be consistent. Should I
> change this one anyway ?
Yes but getValueAndExtraProperties is inside the prototype block, so it is indented by 2 spaces, so its jsdoc is too. You added a new global function, which isn't indented, so its jsdoc shouldn't be either.

> > btw, if the property name has changed, I think we could almost just cancel the pending click, when the name changes, the meaning of the value changes too, and so there are a lot of chances that it won't have swatches anymore, or have them at different locations
> 
> I don't know if we should do this. I know this is an edge case, but the user
> could changed to color to background-color (or border-color to
> border-top-color), and immediatly want to change the color value. 
That's a good point. So you're saying that the advantages of keeping the pending click even when the name changes outweigh the drawback of having a potentially unrelated tooltip appear on click.
I think I agree, so let's go with that.

Comment 17

[Patrick Brosset <:pbro>]

About the generic/specific discussion here: I was looking at the code more and realized one thing (which I should have done long ago): in 'onNameDone', the first thing we do is make sure the name was changed before proceeding, which sounds good, and made me think why do we even have to handle pending clicks since nothing changed and so the DOM shouldn't change either.
But I realized that we have a callback for 'destroy' in the inplace-editor which always calls update, no matter what. So even if we skipped 'onNameDone' if the name hadn't changed, we still update and recreate the whole property.
I tested something quickly, by setting 'this._nameWasUnchanged = true;' in the first 'if' in 'onNameDone', and then on the 'destroy' callback of the name inplace-editor, instead of calling 'update', I call a new function 'onNameDestroyed' which I use to bail out:
  _onNameEditorDestroy: function () {
    if (this._nameWasUnchanged) {
      return;
    }
    this._nameWasUnchanged = false;
    this.update();
  },
This fixes the original bug for me, without requiring handling the pending click. 
This still leaves the bug for when you do change the property name. Same as today. So maybe for this, we could use your pending click mechanism. But at least, I think we should handle the case where the name didn't change much more simply.

Now, to actually answer the question: I do think we should go with the specific code, store data about what was clicked on, a color swatch, a url, etc... and which one. And then, after update (only if the name had changed), try and find it again in order to click on it.

How does that sound?

Comment 18

[Nicolas Chevobbe [:nchevobbe]]

(In reply to Patrick Brosset <:pbro> from comment #17)
> About the generic/specific discussion here: I was looking at the code more
> and realized one thing (which I should have done long ago): in 'onNameDone',
> the first thing we do is make sure the name was changed before proceeding,
> which sounds good, and made me think why do we even have to handle pending
> clicks since nothing changed and so the DOM shouldn't change either.
> But I realized that we have a callback for 'destroy' in the inplace-editor
> which always calls update, no matter what. So even if we skipped
> 'onNameDone' if the name hadn't changed, we still update and recreate the
> whole property.
> I tested something quickly, by setting 'this._nameWasUnchanged = true;' in
> the first 'if' in 'onNameDone', and then on the 'destroy' callback of the
> name inplace-editor, instead of calling 'update', I call a new function
> 'onNameDestroyed' which I use to bail out:
>   _onNameEditorDestroy: function () {
>     if (this._nameWasUnchanged) {
>       return;
>     }
>     this._nameWasUnchanged = false;
>     this.update();
>   },
> This fixes the original bug for me, without requiring handling the pending
> click. 
> This still leaves the bug for when you do change the property name. Same as
> today. So maybe for this, we could use your pending click mechanism. But at
> least, I think we should handle the case where the name didn't change much
> more simply.

If you look at my last patch, I think I handled this. The callback on destroy calls a new function that just take care of the enable/disable checkbox and other data that are hidden when the name is being edited

`
      editableField({
        start: this._onStartEditing,
        element: this.nameSpan,
        done: this._onNameDone,
        destroy: this.updatePropertyState,
        advanceChars: ":",
        contentType: InplaceEditor.CONTENT_TYPES.CSS_PROPERTY,
        popup: this.popup
      });
`
the new function : 

`
  /**
   * Update the visibility of the enable checkbox, the warning indicator and
   * the filter property, as well as the overriden state of the property.
   */
  updatePropertyState: function () {
`

And that seems to be enough, because `onNameDone` will call the update function when the property's name is changed

> Now, to actually answer the question: I do think we should go with the
> specific code, store data about what was clicked on, a color swatch, a url,
> etc... and which one. And then, after update (only if the name had changed),
> try and find it again in order to click on it.
> 
> How does that sound?

Sounds legit, I'll do it that way

Comment 19

[Nicolas Chevobbe [:nchevobbe]]

(In reply to Patrick Brosset <:pbro> from comment #16)
> (In reply to Nicolas Chevobbe [:nchevobbe] from comment #15)
> > https://reviewboard.mozilla.org/r/52632/#review52174
> > 
> > > This jsdoc comment is indented, it should not.
> > 
> > oh, I reproduced what he have in this file (see `_previewValue`,
> > `getValueAndExtraProperties`, ...) in order to be consistent. Should I
> > change this one anyway ?
> Yes but getValueAndExtraProperties is inside the prototype block, so it is
> indented by 2 spaces, so its jsdoc is too. You added a new global function,
> which isn't indented, so its jsdoc shouldn't be either.

Oh, I think I misunderstood what you were saying. I thought you were talking about not indenting comment after line break like

`
   * @param {Node} parentEl
   *        The root node from where we want to get the child element.
`

I do see now that it's the whole block that is indented and shouldn't

Comment 20

[Nicolas Chevobbe [:nchevobbe]]

Comment on attachment 8752560 [details]
Bug 1248274 - Fix click on augmented value elements (swatch, url,…) on the RuleView when property's name is being edited.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/52632/diff/3-4/

Comment 21

[Patrick Brosset <:pbro>]

Comment on attachment 8752560 [details]
Bug 1248274 - Fix click on augmented value elements (swatch, url,…) on the RuleView when property's name is being edited.

https://reviewboard.mozilla.org/r/52632/#review53058

The code looks good to me.
About the test, I think we might need 2 other cases:
- one test where you *don't* change the property name first
- and one test with a value containing a URL.
These should probably go into separate tests though, to avoid risks of having tests running too long on slow VMs. By the way, your test seems rather long. It takes 13s on my machine, and I know that on very slow environments (linux debug e10s, under load), this can easily go above the 45s threshold.
Let's R+ this patch, and maybe as a second patch work on the new tests and split this one in 3.

::: devtools/client/inspector/rules/test/browser_rules_edit-property_10.js:42
(Diff revision 4)
> +function* testColorValueSpanClick(ruleEditor, view) {
> +  info("Test click on color span while editing property name");
> +  let propEditor = ruleEditor.rule.textProps[0].editor;
> +  let colorSpan = propEditor.valueSpan.querySelector(".ruleview-color");
> +
> +  info("Focus the color name space");

I believe you meant "span" instead of "space" here.

::: devtools/client/inspector/rules/test/browser_rules_edit-property_10.js:75
(Diff revision 4)
> +  info("Test shift + click on color swatch while editing property name");
> +  let propEditor = ruleEditor.rule.textProps[1].editor;
> +  let swatchSpan = propEditor.valueSpan.querySelectorAll(
> +    ".ruleview-colorswatch")[2];
> +
> +  info("Focus the background name space");

"span" instead of "space"

::: devtools/client/inspector/rules/views/text-property-editor.js:32
(Diff revision 4)
> +const COLOR_SWATCH_CLASS = "ruleview-colorswatch";
> +const BEZIER_SWATCH_CLASS = "ruleview-bezierswatch";
> +const FILTER_SWATCH_CLASS = "ruleview-filterswatch";
> +const ANGLE_SWATCH_CLASS = "ruleview-angleswatch";
> +
> +const ACTIONABLE_ELEMENTS_SELECTORS = [

Please add a comment before this const explaining what is an "actionable" element.

::: devtools/client/inspector/rules/views/text-property-editor.js:237
(Diff revision 4)
>          }
>        }, false);
>  
> +      // As there is an update on nameContainer blur, we need to remember
> +      // where the user clicked on mousedown (which can trigger the blur) in
> +      // order to fire the click after the update.

This needs to be a little bit clearer. It might not be immediately obvious to people reading the code that the update actually re-creates the markup and that therefore the "normal" click doesn't go through.

::: devtools/client/inspector/rules/views/text-property-editor.js:427
(Diff revision 4)
> +        let elements = this.valueSpan.querySelectorAll(selector);
> +        elToClick = elements[index];

Maybe only one line here:
`elToClick = this.valueSpan.querySelectorAll(selector)[index];`

Comment 22

[Nicolas Chevobbe [:nchevobbe]]

Attachment: Bug 1248274 - Split long test and add new ones to make sure the bug is fixed.

Review commit: https://reviewboard.mozilla.org/r/56816/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/56816/

Comment 23

[Nicolas Chevobbe [:nchevobbe]]

Comment on attachment 8752560 [details]
Bug 1248274 - Fix click on augmented value elements (swatch, url,…) on the RuleView when property's name is being edited.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/52632/diff/4-5/

Comment 24

[Nicolas Chevobbe [:nchevobbe]]

https://reviewboard.mozilla.org/r/56816/#review53528

::: devtools/client/inspector/rules/test/browser_rules_edit-property_10.js:41
(Diff revision 1)
> -  info("Modify the property to border-color to trigger the " +
> -    "property-value-updated event");
> -  editor.input.value = "border-color";
> +  // We add a click event to make sure the color span won't be cleared
> +  // on nameSpan blur (which would lead to the click event not being triggered)
> +  let onColorSpanClick = new Promise(function (resolve, reject) {
> +    let handler = () => {
> +      colorSpan.removeEventListener("click", handler);
> +      resolve();
> +    };
> +    colorSpan.addEventListener("click", handler);
> +  });
>  
> -  let onPropertyValueUpdate = view.once("property-value-updated");
> +  // The property-value-updated is emitted when the valueSpan markup is being
> +  // re-populated, which should not be the case when not modifying the property name
> +  let onPropertyValueUpdated = function () {
> +    ok(false, "The \"property-value-updated\" should not be emitted");
> +  };
> +  view.on("property-value-updated", onPropertyValueUpdated);

Really not sure about those 2 things. 
I thought about having a timeout for `property-value-updated` only, which whould fail if the event is emitted before the time's out, but I found it risky (and jryans agreed it could lead to problems).

It works well as is, but I'm really open to any smarter solution to test this.

Comment 25

[Patrick Brosset <:pbro>]

Comment on attachment 8758563 [details]
Bug 1248274 - Split long test and add new ones to make sure the bug is fixed.

https://reviewboard.mozilla.org/r/56816/#review54288

Looks good.

::: devtools/client/inspector/rules/test/browser.ini:117
(Diff revision 1)
>  [browser_rules_edit-property_10.js]
> +[browser_rules_edit-property_11.js]
> +[browser_rules_edit-property_12.js]
> +[browser_rules_edit-property_13.js]

Could you rename those tests to:

browser_rules_edit-value-after-name_01/2/3/4.js

We should refrain from having too many tests with the same name and a number at the end. These 4 ones are for a specific case only, so let's make it explicit in the name (and then, it turns out we have 4 sub-cases to avoid long tests, so it's fine to have 1,2,3,4 for those, but at least the name before those is self explanatory).

::: devtools/client/inspector/rules/test/browser_rules_edit-property_10.js:42
(Diff revision 1)
> -    "property-value-updated event");
> -  editor.input.value = "border-color";
> +  // on nameSpan blur (which would lead to the click event not being triggered)
> +  let onColorSpanClick = new Promise(function (resolve, reject) {
> +    let handler = () => {
> +      colorSpan.removeEventListener("click", handler);
> +      resolve();
> +    };
> +    colorSpan.addEventListener("click", handler);
> +  });

You're essentially re-implementing `once` here. Use `once` to make it much shorter:

```
let onColorSpanClick = once(colorSpan, "click");
```

::: devtools/client/inspector/rules/test/browser_rules_edit-property_10.js:61
(Diff revision 1)
> -  info("wait for the property value to be updated");
> -  yield onPropertyValueUpdate;
> +  info("wait for the click event on the color span");
> +  yield onColorSpanClick;

Ah ok, I see. You're listening to the click because:

- either the valueSpan is updated and the click will happen after this
- or it's not updated (which we except) and the click happens right away

In both cases, waiting for the click event tells you that the whole thing is done, and it's safe to test that the property-value-updated wasn't emitted at that point.
Smart.

::: devtools/client/inspector/rules/test/browser_rules_edit-property_11.js:33
(Diff revision 1)
> +  yield testColorSwatchShiftClick(ruleEditor, view);
> +});
> +
> +function* testColorSwatchShiftClick(ruleEditor, view) {

Because this test now does only 1 thing (good!) and therefore has only function function, I suggest just putting everything into `add_task`, So, no need for these 4 lines.

::: devtools/client/inspector/rules/test/browser_rules_edit-property_12.js:32
(Diff revision 1)
> +  yield testColorSwatchClick(ruleEditor, view);
> +});
> +
> +function* testColorSwatchClick(ruleEditor, view) {

Because this test now does only 1 thing (good!) and therefore has only function function, I suggest just putting everything into `add_task`, So, no need for these 4 lines.

::: devtools/client/inspector/rules/test/browser_rules_edit-property_13.js:26
(Diff revision 1)
> +  yield testBackgroundImageClick(ruleEditor, view);
> +});
> +
> +function* testBackgroundImageClick(ruleEditor, view) {

Because this test now does only 1 thing (good!) and therefore has only function function, I suggest just putting everything into `add_task`, So, no need for these 4 lines.

Comment 26

[Nicolas Chevobbe [:nchevobbe]]

Comment on attachment 8752560 [details]
Bug 1248274 - Fix click on augmented value elements (swatch, url,…) on the RuleView when property's name is being edited.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/52632/diff/5-6/

Comment 27

[Nicolas Chevobbe [:nchevobbe]]

Comment on attachment 8758563 [details]
Bug 1248274 - Split long test and add new ones to make sure the bug is fixed.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/56816/diff/1-2/

Comment 28

[Nicolas Chevobbe [:nchevobbe]]

Comment on attachment 8752560 [details]
Bug 1248274 - Fix click on augmented value elements (swatch, url,…) on the RuleView when property's name is being edited.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/52632/diff/6-7/

Comment 29

[Nicolas Chevobbe [:nchevobbe]]

Comment on attachment 8758563 [details]
Bug 1248274 - Split long test and add new ones to make sure the bug is fixed.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/56816/diff/2-3/

Comment 30

[Nicolas Chevobbe [:nchevobbe]]

First TRY run was almost green (https://treeherder.mozilla.org/#/jobs?repo=try&revision=a3ee6b3f5c6e), except known intermittents and one ESLint failure on a file of my first patch.
I fixed the ESLint error and pushed my patch again, to reviewboard (https://reviewboard.mozilla.org/r/52632/diff/6-7/) and TRY for ESLint check only (https://treeherder.mozilla.org/#/jobs?repo=try&revision=2dafbe0dda1a).

Comment 31

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/fx-team/rev/4168becc7a7a
Fix click on augmented value elements (swatch, url,…) on the RuleView when property's name is being edited. r=pbro
https://hg.mozilla.org/integration/fx-team/rev/46b1ef3d9e4f
Split long test and add new ones to make sure the bug is fixed. r=pbro

Comment 32

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/4168becc7a7a
https://hg.mozilla.org/mozilla-central/rev/46b1ef3d9e4f

Comment 33

[Nicolas Chevobbe [:nchevobbe]]

https://reviewboard.mozilla.org/r/56816/#review55014

Comment 34

[Nicolas Chevobbe [:nchevobbe]]

Comment on attachment 8758563 [details]
Bug 1248274 - Split long test and add new ones to make sure the bug is fixed.

https://reviewboard.mozilla.org/r/56816/#review55016

Comment 36

[arni2033]

Affected:   Win7_64, Nightly 49, 32bit, ID 20160526082509  (2016-05-26)
Fixed on:   Win7_64, Nightly 50, 32bit, ID 20160606131341  (2016-06-06)