1202458 - [markup-view] Collapsing an element which has only text child by clicking on another element may move event target from under the mouse cursor

Status: RESOLVED FIXED

(DevTools :: Inspector, defect, P2)

Description

[arni2033]

Attachment: testpage - [markup-view] Collapsing an element which has only text child by clicking on another element may move event target from under the mouse cursor.htm

STR:   (Win7_64, Nightly 43, 32bit, ID 20150901030226, new profile, safe mode)
1. Open attached page
2. Right-click gray area, click "Inspect Element".
3. Detach devtools into separate window, maximize that window
4. Hide sidebar   [actually it's super easy to reproduce, I just needed reliable STR]
5. Click the 1st <p> element in markup-view
6. Move mouse to the second <p> element and click it

Result:       First <p> element collapsed, and second <p> element was moved from under mouse cursor
Expectations: Second <p> element should stay still

Note: If this bug will be regarded as valid, and will be fixed, then bug 1202179 would be not so bad and maybe could be closed

Comment 1

[arni2033]

I personally think that this could be solved in 2 ways:
   1) Return to old approach of displaying text nodes as children (with some modifications): If element has only 1 children which is text node, then there should be a dropmarker near that element. When you click the dropmarker, text child should expand/collapse. And never collapse on blur.       If element has many children, and 1 of them is text node, then there should be a dropmarker near that text node.
   2) Modify all DoubleClick actions to be triggered by single click, like I said in bug 1197252. That will make some things easier, but in this case element should be scrolled into view. This solution interferes with bug 1197267.

Comment 2

[Julian Descottes [:jdescottes]]

Inspector bug triage. Filter on CLIMBING SHOES.
P2 because it is a very easy to reproduce / frequent issue

Comment 3

[Patrick Brosset <:pbro>]

We discussed about this bug today (not specifically, but the general issue of having the layout move when the text expands/contracts). We agreed that 2 things would generally help:

- We should increase the threshold for using an ellipsis on text nodes in the markup-view a lot. So that in most cases, selecting the node doesn't impact the layout.
- And, we should inline single text nodes (in their parent elements) only if they are shorter than a certain length. If a text node is too long, then we should consider it as a separate node in the markup-view.

Comment 4

[Julian Descottes [:jdescottes]]

Attachment: Bug 1202458 - part4: update inspector actor&front tests after removing shortValue;

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

Comment 5

[Julian Descottes [:jdescottes]]

Comment on attachment 8755491 [details]
Bug 1202458 - part4: update inspector actor&front tests after removing shortValue;

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

Comment 6

[Julian Descottes [:jdescottes]]

Comment on attachment 8755491 [details]
Bug 1202458 - part4: update inspector actor&front tests after removing shortValue;

Patrick: I tried two different strategies to fix this Bug and I could use guidance here.

In my first attempt, since all text nodes are now displayed with all their text content, I stopped using string actors and always passed the complete nodeValue to the nodeFront. This allows to simplify a bit the implementation. However, it makes it harder to keep a decent backward compatibility. And also I was not sure that removing the string actors (and consequently passing bigger forms) was a good idea.

That's why I came up with the version attached here. It is closer to the existing implementation, still uses string actors, and connecting to an older debuggee results in having all text nodes treated as children (no inline text nodes).

Tests still need to be updated/extended, but can you let me know what you think about the approach? Thanks!

Comment 7

[Julian Descottes [:jdescottes]]

Comment on attachment 8755491 [details]
Bug 1202458 - part4: update inspector actor&front tests after removing shortValue;

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

Comment 8

[Julian Descottes [:jdescottes]]

Comment on attachment 8755491 [details]
Bug 1202458 - part4: update inspector actor&front tests after removing shortValue;

This patch should apply cleanly on the current fx-team.

Also changed it according to our previous discussion.

Comment 9

[Patrick Brosset <:pbro>]

As discussed on IRC:
Let's try and keep the string actors for easier backward compatibility but also send the full string in the form so that you don't need the extra round trip in case the server is recent enough to send it.
I'll take a look at the code and try it out locally soon (hopefully later today, if not tomorrow).

Comment 10

[Patrick Brosset <:pbro>]

(In reply to Julian Descottes [:jdescottes] from comment #6)
> connecting to an
> older debuggee results in having all text nodes treated as children (no
> inline text nodes).
I think that is fine. Connecting a toolbox to a server that is older is enough of an edge case that we don't need to worry about small behavior changes, as long as the panel still works and the functionality somehow works. Especially because that simplifies dealing with backward compatibility.

Comment 11

[Patrick Brosset <:pbro>]

Have we considered increasing the 50 characters threshold? It feels to me like 80 or even 100 would work nicely too. I guess we could look at our screen sizes stats, infer a size for the markup-view (assuming that the firefox window is maximized, and that the sidebar has its default width), and deduce how many characters fit on a line so that something like
<p>Lorem ipsum dolor sit amet, consectetur adipisicing elit, sed do eiusmod</p>
wouldn't need to wrap on two lines.
(not that wrapping is a problem, but I tend to think that if we inline text nodes, we might as well do it only for text nodes that actually fit nicely on a line).
Could be that 50 is the right value, just suggesting that we look at alternatives. The bigger we can make it (while still looking nice), the less users will need to expand nodes.

Comment 12

[Patrick Brosset <:pbro>]

https://reviewboard.mozilla.org/r/54630/#review52290

::: devtools/client/inspector/markup/markup.js:2890
(Diff revision 3)
>      this._selected = value;
>      this.update();
>    },
>  
>    update: function () {
> -    if (!this.selected || !this.node.incompleteValue) {
> +    if (this.node.shortValue) {

This is only for old debuggees, right? 
Can you add a comment about this in the code. It helps having comments around explaining what is specifically here for backward compatibility reasons, so one day when we can get rid of it, it's easier to find.

::: devtools/shared/fronts/inspector.js:328
(Diff revision 3)
>      }
>      return true;
>    },
>  
>    getNodeValue: custom(function () {
> -    if (!this.incompleteValue) {
> +    if (this._form.nodeValue === null) {

Same comment about adding a comment about backward compatibility here.

::: devtools/shared/fronts/inspector.js:331
(Diff revision 3)
> -      return delayedResolve(new ShortLongString(this.shortValue));
> -    }
> -
> -    return this._getNodeValue();
> +      return this._getNodeValue();
> +    }
> +    return delayedResolve(new ShortLongString(this._form.nodeValue));

Not sure why we still use delayedResolve in this file. We should really just return a resolved promise (especially that we'll have to get rid of `Services.tm.mainThread.dispatch` for dt.html), but this isn't related to your patch.

Comment 13

[Patrick Brosset <:pbro>]

Comment on attachment 8755491 [details]
Bug 1202458 - part4: update inspector actor&front tests after removing shortValue;

I like it. It looks like backward compatibility is handled correctly (I tested it locally too), and the code changes are pretty clear.

In terms of performance, I don't see a problem. If I understand correctly we only inline text nodes if they're below the limit, so we only send at most 50 characters of nodeValue.

Comment 14

[Julian Descottes [:jdescottes]]

Attachment: MozReview Request: Bug 1202458 - part2: fix eslint errors in string actor;r?pbro

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

Comment 15

[Julian Descottes [:jdescottes]]

Attachment: MozReview Request: Bug 1202458 - part3: rename ShortLongString to SimpleStringFront;r?pbro

The current name ShortLongString doesn't reflect the current usage of
this class. When looking at the few clients of this class, the reason
for using it is that the string is already accessible on the client
and does not need to be fetched from the server, while still keeping
the same interface as the LongStringFront.

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

Comment 16

[Julian Descottes [:jdescottes]]

Comment on attachment 8755491 [details]
Bug 1202458 - part4: update inspector actor&front tests after removing shortValue;

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

Comment 17

[Julian Descottes [:jdescottes]]

Comment on attachment 8755491 [details]
Bug 1202458 - part4: update inspector actor&front tests after removing shortValue;

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

Comment 18

[Julian Descottes [:jdescottes]]

Comment on attachment 8758311 [details]
MozReview Request: Bug 1202458 - part2: fix eslint errors in string actor;r?pbro

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

Comment 19

[Julian Descottes [:jdescottes]]

Comment on attachment 8758312 [details]
MozReview Request: Bug 1202458 - part3: rename ShortLongString to SimpleStringFront;r?pbro

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

Comment 20

[Julian Descottes [:jdescottes]]

https://reviewboard.mozilla.org/r/54630/#review52290

> This is only for old debuggees, right? 
> Can you add a comment about this in the code. It helps having comments around explaining what is specifically here for backward compatibility reasons, so one day when we can get rid of it, it's easier to find.

shortValue is actually no longer accessible on the NodeFront, I should have removed this branch! Thanks.

> Same comment about adding a comment about backward compatibility here.

Done!

> Not sure why we still use delayedResolve in this file. We should really just return a resolved promise (especially that we'll have to get rid of `Services.tm.mainThread.dispatch` for dt.html), but this isn't related to your patch.

I tried removing it in the current patch. Everything seems to work fine locally, we'll see if anything pops up during try tests. 

If I'm not mistaken, the front code is already running on the main thread? So this should be the equivalent to a promise.resolve.

Comment 21

[Julian Descottes [:jdescottes]]

Comment on attachment 8755491 [details]
Bug 1202458 - part4: update inspector actor&front tests after removing shortValue;

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

Comment 22

[Julian Descottes [:jdescottes]]

Comment on attachment 8758311 [details]
MozReview Request: Bug 1202458 - part2: fix eslint errors in string actor;r?pbro

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

Comment 23

[Julian Descottes [:jdescottes]]

Comment on attachment 8758312 [details]
MozReview Request: Bug 1202458 - part3: rename ShortLongString to SimpleStringFront;r?pbro

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

Comment 24

[Julian Descottes [:jdescottes]]

Comment on attachment 8755491 [details]
Bug 1202458 - part4: update inspector actor&front tests after removing shortValue;

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

Comment 25

[Julian Descottes [:jdescottes]]

Comment on attachment 8758311 [details]
MozReview Request: Bug 1202458 - part2: fix eslint errors in string actor;r?pbro

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

Comment 26

[Julian Descottes [:jdescottes]]

Comment on attachment 8758312 [details]
MozReview Request: Bug 1202458 - part3: rename ShortLongString to SimpleStringFront;r?pbro

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

Comment 27

[Patrick Brosset <:pbro>]

Comment on attachment 8758311 [details]
MozReview Request: Bug 1202458 - part2: fix eslint errors in string actor;r?pbro

https://reviewboard.mozilla.org/r/56590/#review53734

Thanks for the eslint cleanup. This looks good to me.
Just one thing, in order to avoid new eslint errors in the future, you could add this to our .eslintignore file:
!devtools/server/actors/string.js

(we do have this already for a few other actors, so it makes sense to do it file by file I believe).

Comment 28

[Patrick Brosset <:pbro>]

Comment on attachment 8758312 [details]
MozReview Request: Bug 1202458 - part3: rename ShortLongString to SimpleStringFront;r?pbro

https://reviewboard.mozilla.org/r/56592/#review53736

If I didn't miss anything, this is a client-side only change, so there are no compatibility issues related to this change. Seems good to me. Thanks for making the name more self explanatory.

Comment 29

[Patrick Brosset <:pbro>]

Comment on attachment 8755491 [details]
Bug 1202458 - part4: update inspector actor&front tests after removing shortValue;

https://reviewboard.mozilla.org/r/54630/#review53748

Ship it!

::: devtools/client/inspector/markup/test/browser_markup_textcontent_display.js:27
(Diff revision 7)
> +  let body = yield getContainerForSelector("body", inspector);
> +  ok(body.expanded, "Body should be expanded by default on test startup");

Is this check really needed for this test? It seems unrelated.

::: devtools/client/inspector/markup/test/browser_markup_textcontent_display.js:38
(Diff revision 7)
> +    inline: true,
> +    value: "Short text",
> +  });
> +
> +  info("Test node containing a long text, long text nodes are not inlined.");
> +  yield checkNode(inspector, testActor, {

In most other tests that run several times the same code for several cases, we usually extract the test data into an array and a single test look in the code.
But it's not a big deal. I'll let you choose whichever you like best:

```
let TEST_DATA = [{
  desc: "Test node containing a short text, short text nodes can be inlined."
  selector: "#shorttext",
  inline: true,
  value: "Short text"
}, {
  desc: "Test node containing a long text, long text nodes are not inlined.",
  selector: "#longtext",
  inline: false,
  value: LONG_VALUE
}, {
  ...
}];

add_task(function* () {
  let {inspector, testActor} = yield openInspectorForURL(TEST_URL);
  for (let {desc, selector, inline, value} of TEST_DATA) {
    info(desc);
    yield checkNode(inspector, testActor, {selector, inline, value});
  }
});
```

::: devtools/client/inspector/markup/test/browser_markup_textcontent_edit_01.js:19
(Diff revision 7)
>  
>    info("Expanding all nodes");
>    yield inspector.markup.expandAll();
>    yield waitForMultipleChildrenUpdates(inspector);
>  
>    yield editContainer(inspector, testActor, {

Ah, I see that this test doesn't do what I said you should do in the new test :)
So, again, up to you to choose if you want to move the test data in an array or not for your new test.

Comment 30

[Julian Descottes [:jdescottes]]

Comment on attachment 8755491 [details]
Bug 1202458 - part4: update inspector actor&front tests after removing shortValue;

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

Comment 31

[Julian Descottes [:jdescottes]]

Comment on attachment 8758311 [details]
MozReview Request: Bug 1202458 - part2: fix eslint errors in string actor;r?pbro

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

Comment 32

[Julian Descottes [:jdescottes]]

Comment on attachment 8758312 [details]
MozReview Request: Bug 1202458 - part3: rename ShortLongString to SimpleStringFront;r?pbro

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

Comment 33

[Julian Descottes [:jdescottes]]

https://reviewboard.mozilla.org/r/54630/#review53748

Thanks for the reviews! Rebased and push to try again: https://treeherder.mozilla.org/#/jobs?repo=try&revision=726904d90ddb

> Is this check really needed for this test? It seems unrelated.

Removed it, thanks.

> In most other tests that run several times the same code for several cases, we usually extract the test data into an array and a single test look in the code.
> But it's not a big deal. I'll let you choose whichever you like best:
> 
> ```
> let TEST_DATA = [{
>   desc: "Test node containing a short text, short text nodes can be inlined."
>   selector: "#shorttext",
>   inline: true,
>   value: "Short text"
> }, {
>   desc: "Test node containing a long text, long text nodes are not inlined.",
>   selector: "#longtext",
>   inline: false,
>   value: LONG_VALUE
> }, {
>   ...
> }];
> 
> add_task(function* () {
>   let {inspector, testActor} = yield openInspectorForURL(TEST_URL);
>   for (let {desc, selector, inline, value} of TEST_DATA) {
>     info(desc);
>     yield checkNode(inspector, testActor, {selector, inline, value});
>   }
> });
> ```

I switched to this pattern for consistency, thanks for the reminder.

Comment 34

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/fx-team/rev/f3bdf3ebaf4c
part1: inline text nodes in markupview only if they are short enough;r=pbro
https://hg.mozilla.org/integration/fx-team/rev/49d7963e43d8
part2: fix eslint errors in string actor;r=pbro
https://hg.mozilla.org/integration/fx-team/rev/e74192b1a9c8
part3: rename ShortLongString to SimpleStringFront;r=pbro

Comment 35

[Carsten Book [:Tomcat]]

sorry had to back this out for test failures like https://treeherder.mozilla.org/logviewer.html#?job_id=9707916&repo=fx-team

Comment 36

[Pulsebot]

Backout by [e-mail]:
https://hg.mozilla.org/integration/fx-team/rev/4f76ef821616
Backed out changeset e74192b1a9c8 
https://hg.mozilla.org/integration/fx-team/rev/e75f2d129de0
Backed out changeset 49d7963e43d8 for test failures in test_inspector-mutations-value.html

Comment 37

[Pulsebot]

Backout by [e-mail]:
https://hg.mozilla.org/integration/fx-team/rev/c26aac8cf086
Backed out changeset f3bdf3ebaf4c for test failures in test_inspector-mutations-value.html - missed that changeset in the backout before, sorry

Comment 38

[Julian Descottes [:jdescottes]]

Comment on attachment 8755491 [details]
Bug 1202458 - part4: update inspector actor&front tests after removing shortValue;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/54630/diff/8-9/

Comment 39

[Julian Descottes [:jdescottes]]

Comment on attachment 8755491 [details]
Bug 1202458 - part4: update inspector actor&front tests after removing shortValue;

Thanks for handling the backout! Two test failures from the oth test suite which I forgot to include in my previous tests.

pbro: Not sure why mozreview carried over an r+ here, so adding it here.

Comment 40

[Patrick Brosset <:pbro>]

Comment on attachment 8755491 [details]
Bug 1202458 - part4: update inspector actor&front tests after removing shortValue;

https://reviewboard.mozilla.org/r/54630/#review54316

::: devtools/server/tests/mochitest/test_inspector-mutations-value.html:93
(Diff revision 9)
> +  return new Promise(resolve => {
> +    front.getNodeValue().then(longstring => {
> +      return longstring.string();
> +    }).then(str => {
> +      is(str, expectedValue, "Node value is as expected");
> +      resolve();
> +    })
> +  });

front.getNodeValue() already returns a promise, no need to wrap this in `new Promise`.

Comment 41

[Julian Descottes [:jdescottes]]

https://reviewboard.mozilla.org/r/54630/#review54316

> front.getNodeValue() already returns a promise, no need to wrap this in `new Promise`.

Thanks for the review! Removed the useless "new Promise" wrapper.

Comment 42

[Julian Descottes [:jdescottes]]

Landed:

https://hg.mozilla.org/integration/fx-team/rev/00ee1688f8a2
https://hg.mozilla.org/integration/fx-team/rev/e3622cb1ef42
https://hg.mozilla.org/integration/fx-team/rev/96f3ba6f4096
https://hg.mozilla.org/integration/fx-team/rev/c60e3877ccf6

Comment 43

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/00ee1688f8a2
https://hg.mozilla.org/mozilla-central/rev/e3622cb1ef42
https://hg.mozilla.org/mozilla-central/rev/96f3ba6f4096
https://hg.mozilla.org/mozilla-central/rev/c60e3877ccf6

Comment 44

[Julian Descottes [:jdescottes]]

Attachment: textnodes-collapsing.gif

Comment 45

[Mohammad Maruf Rahman]

I have reproduced this bug with Nightly 43.0a1 (2015-09-07) on Windows 7, 64 Bit!

This bug's fix is verified with latest Beta!

Build  ID     20160811031722
User Agent    Mozilla/5.0 (Windows NT 6.1; Win64; x64; rv:49.0) Gecko/20100101 Firefox/49.0

[testday-20160812]