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

RESOLVED FIXED in Firefox 49

Status

()

Firefox
Developer Tools: Inspector
P2
normal
RESOLVED FIXED
3 years ago
2 years ago

People

(Reporter: arni2033, Assigned: jdescottes)

Tracking

Trunk
Firefox 49
Points:
---

Firefox Tracking Flags

(firefox43 affected, firefox49 fixed)

Details

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(3 attachments, 2 obsolete attachments)

(Reporter)

Description

3 years ago
Created attachment 8657873 [details]
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
(Reporter)

Comment 1

3 years ago
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.
See Also: → bug 1197252
(Reporter)

Updated

3 years ago
See Also: → bug 1197267
(Reporter)

Updated

2 years ago
Has STR: --- → yes
(Reporter)

Updated

2 years ago
No longer blocks: 1202179
See Also: → bug 1202179
(Reporter)

Updated

2 years ago
See Also: → bug 1248296
Inspector bug triage. Filter on CLIMBING SHOES.
P2 because it is a very easy to reproduce / frequent issue
Priority: -- → P2
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.
Blocks: 1264624
Assignee: nobody → jdescottes
Status: NEW → ASSIGNED
Created attachment 8755491 [details]
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 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 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!
Attachment #8755491 - Flags: feedback?(pbrosset)
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/
Attachment #8755491 - Flags: feedback?(pbrosset)
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.
Attachment #8755491 - Flags: feedback?(pbrosset)
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).
(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.
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.
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 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.
Attachment #8755491 - Flags: feedback?(pbrosset) → feedback+
Created attachment 8758311 [details]
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/
Attachment #8755491 - Attachment description: MozReview Request: Bug 1202458 - WIP Collapse text nodes in markup view if text too long → MozReview Request: Bug 1202458 - part1: inline text nodes in markupview only if they are short enough
Attachment #8755491 - Flags: feedback+
Created attachment 8758312 [details]
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 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 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 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 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/
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 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 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 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 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/
Attachment #8755491 - Attachment description: MozReview Request: Bug 1202458 - part1: inline text nodes in markupview only if they are short enough → MozReview Request: Bug 1202458 - part1: inline text nodes in markupview only if they are short enough;r?pbro
Attachment #8758311 - Attachment description: MozReview Request: Bug 1202458 - part2: fix eslint errors in string actor → MozReview Request: Bug 1202458 - part2: fix eslint errors in string actor;r?pbro
Attachment #8758312 - Attachment description: MozReview Request: Bug 1202458 - part3: rename ShortLongString to SimpleStringFront → MozReview Request: Bug 1202458 - part3: rename ShortLongString to SimpleStringFront;r?pbro
Attachment #8755491 - Flags: review?(pbrosset)
Attachment #8758311 - Flags: review?(pbrosset)
Attachment #8758312 - Flags: review?(pbrosset)
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 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 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).
Attachment #8758311 - Flags: review?(pbrosset) → review+
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.
Attachment #8758312 - Flags: review?(pbrosset) → review+
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.
Attachment #8755491 - Flags: review?(pbrosset) → review+
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 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 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/
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

2 years ago
Pushed by jdescottes@mozilla.com:
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
sorry had to back this out for test failures like https://treeherder.mozilla.org/logviewer.html#?job_id=9707916&repo=fx-team
Flags: needinfo?(jdescottes)

Comment 36

2 years ago
Backout by cbook@mozilla.com:
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

2 years ago
Backout by cbook@mozilla.com:
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 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/
Attachment #8755491 - Attachment description: MozReview Request: Bug 1202458 - part1: inline text nodes in markupview only if they are short enough;r?pbro → Bug 1202458 - part4: update inspector actor&front tests after removing shortValue;
Attachment #8758311 - Attachment is obsolete: true
Attachment #8758312 - Attachment is obsolete: true
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.
Flags: needinfo?(jdescottes)
Attachment #8755491 - Flags: review+ → review?(pbrosset)
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`.
Attachment #8755491 - Flags: review?(pbrosset) → review+
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 43

2 years ago
bugherder
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
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago
status-firefox49: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 49
No longer blocks: 1264624
Created attachment 8760795 [details]
textnodes-collapsing.gif
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]
You need to log in before you can comment on or make changes to this bug.