1281051 - Remove text-node rep

Status: RESOLVED FIXED

(DevTools :: Shared Components, defect, P1)

Description

[Lin Clark [:linclark]]

As explained in Bug 1264699: 

We believe it was added in an initial version of the HTTP inspector when HTML output was also supported. However, since that support was removed, this probably isn't used.

Comment 1

[Lin Clark [:linclark]]

Attachment: Bug1281051.patch

Comment 2

[Jan Honza Odvarko [:Honza] (always need-info? me)]

Attachment: text-node-rep.png

I've been testing this and the rep is used in the DOM panel to render text nodes.

I am attaching a screenshot, see the `_b` variable.

Value of the text node in DOM panel is rendered using 'tiny' mode (at the top in the screenshot). I also tried how it looks like if 'long' mode is used (at the bottom in the screenshot).

The long mode uses markup to render the element and its attributes and is intended for the Console panel. Firebug has been using it for the HTML/Inspector panel as well and we should do it too as soon as the Inspector panel is ready to be entirely rendered using Element Reps.

We should add new CSS to make color syntax look nice (e.g. brackets, the equal sign, quotes, tag name, attr name, attr value) at some point.

Honza

Comment 3

[Lin Clark [:linclark]]

I'm not sure that this is how we want it to work.

For example, the console currently outputs this for a text node:

> #text "test string"

Chrome's console outputs

> "test string"

My understanding is that the inspector would continue to use the editor.

For DOM Panel (using tiny mode), is there any difference between the way this rep handles it and what would happen if it fell back to the default?

Comment 4

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/fx-team/rev/c4096b7ccbe6
Remove text-node rep. r=Honza

Comment 5

[Ryan VanderMeulen [:RyanVM]]

(In reply to Pulsebot from comment #4)
> Pushed by [e-mail]:
> https://hg.mozilla.org/integration/fx-team/rev/c4096b7ccbe6
> Remove text-node rep. r=Honza

This was actually bug 1264679.

Comment 6

[Jan Honza Odvarko [:Honza] (always need-info? me)]

Attachment: text-node-default.png

(In reply to Lin Clark [:linclark] from comment #3)
> I'm not sure that this is how we want it to work.
> 
> For example, the console currently outputs this for a text node:
> 
> > #text "test string"
> 
> Chrome's console outputs
> 
> > "test string"
You are right text-node is special and it doesn't need markup. This should be fixed.
I am more thinking about regular DOM elements here. 

> My understanding is that the inspector would continue to use the editor.
I can understand that refactoring of the current inspector (markupview.html) can be quite a lot of work and not priority now, but... From the Reps perspective, all you see in the Inspector are just elements. So, for example, if you log an (regular) element into the Console panel, you'll see:

<div id="text-content">

(btw. I already have `Element` rep and other DOM type reps on my machine and I am just waiting for the right time to provide a patch).

If you go to the Inspector panel to check out the same element, you'll see:

<div id="text-content">Some Text</div>

Isn't that quite the same? (the console version rendered in 'short' mode and the Inspector version rendered in 'long' mode). Not using reps for the Inspector panel leads to code duplication. There is a lot more we can do with Reps and what we have now (primitive types) is just a beginning. Having support for complex data structures will be a lot more fun!

> For DOM Panel (using tiny mode), is there any difference between the way
> this rep handles it and what would happen if it fell back to the default?
Yes, see my attached screenshot. Currently, you can see the actual text value. Default renders just `Type` and the user needs to expand it and lookup the value in property list. It's neat. Is there a reason why we should remove it?

Honza

Comment 7

[Lin Clark [:linclark]]

I think this is probably going to need further discussion, especially once we start incorporating reps into debugger and console. I'd say it's probably best to deprioritize this issue for now.

Comment 8

[Jan Honza Odvarko [:Honza] (always need-info? me)]

Comment on attachment 8763682 [details] [diff] [review]
Bug1281051.patch

Sounds good to me (I am removing the review request for now).

Honza

Comment 9

[Jan Honza Odvarko [:Honza] (always need-info? me)]

(In reply to Lin Clark [:linclark] from comment #7)
> I think this is probably going to need further discussion, especially once
> we start incorporating reps into debugger and console. I'd say it's probably
> best to deprioritize this issue for now.
Lin, I'd like to fix this report.
Are there any new requirements at this point?
How the rendering should look like?
Honza

Comment 10

[Lin Clark [:linclark]]

For text node, I'd say let's go with whatever Chrome does.

Comment 11

[Patrick Brosset <:pbro>]

My 2 cents:

I see both Element rep and TextNode rep being mentioned here. I think we need to keep them separated.
Even though text nodes and element nodes both exist in the DOM, they are quite different, and users expect them to look different in devtools.
I agree with the larger point that Honza is making that we should reuse code as much as possible, so as soon as we have these shareable reps for the console, we should reuse them in the debugger too and, in time, in the inspector panel too.
But in terms of implementation text nodes and element nodes will require 2 separate reps. They just look and behave completely differently. One has attributes and tagnames that can be edited, the other doesn't. One has angle brackets around it, the other doesn't, etc.

Once (if) we refactor the markup-view to react, it'd be really cool to reuse these reps.

2 random thoughts:
- the element reps should also be able to handle pseudo-elements too: ::before and ::after
- both text nodes and element nodes can be highlighted in the page. In fact I'm about to land a patch in bug 1304679 that will make it possible to highlight text nodes just we do element nodes.
So, in effect, what that means is that as you hover over an element node in the console, you see it highlighted in the page, well soon, we'll be able to do the same for text nodes.
Reps should handle this at some stage (if they don't already).

Comment 12

[Jan Honza Odvarko [:Honza] (always need-info? me)]

Attachment: bug1281051.patch

Attaching a patch for this:

- There will be differences between the default (String) rep and TextNode (e.g. the highlighting on mouseover) so, I am keeping it in.
- The format of the text (e.g. new lines or tabs) is fixed (just like Chrome does)
- I also kept the '#text' title since it's used to show the Variables view (by clicking on it)
- Color of the text value is the same as for String rep.

Tested with the new front end.

Honza

Comment 13

[Lin Clark [:linclark]]

Should there be a test for this?

Comment 14

[Jan Honza Odvarko [:Honza] (always need-info? me)]

Attachment: bug1281051.patch

(In reply to Lin Clark [:linclark] from comment #13)
> Should there be a test for this?
Good point, I extended the existing test for text-node.
Patch updated.

Honza

Comment 15

[Lin Clark [:linclark]]

Comment on attachment 8798479 [details] [diff] [review]
bug1281051.patch

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

::: devtools/client/shared/components/reps/text-node.js
@@ +47,5 @@
>        if (mode == "short" || mode == "tiny") {
>          return (
>            DOM.span({className: "objectBox objectBox-textNode"},
>              this.getTitle(grip),
> +            DOM.pre({className: "nodeValue"},

It's not clear to me why this is a pre. It doesn't look like it is in the current console, or in Chrome.

::: devtools/client/shared/components/test/mochitest/test_reps_text-node.html
@@ +48,5 @@
>        "TextNode rep has expected class names");
>      is(renderedComponent.textContent, `"hello world"`,
>        "TextNode rep has expected text content");
> +
> +    // Test rendering with EOL character.

The existing test should be rewritten to cover all of the output. That includes the #text part before hand.

Also, why is renderedComponent2 used here? This could be broken up into two separate tasks or functions.

Comment 16

[Jan Honza Odvarko [:Honza] (always need-info? me)]

Attachment: bug1281051.patch

(In reply to Lin Clark [:linclark] from comment #15)
> Comment on attachment 8798479 [details] [diff] [review]
> bug1281051.patch
> 
> Review of attachment 8798479 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: devtools/client/shared/components/reps/text-node.js
> @@ +47,5 @@
> >        if (mode == "short" || mode == "tiny") {
> >          return (
> >            DOM.span({className: "objectBox objectBox-textNode"},
> >              this.getTitle(grip),
> > +            DOM.pre({className: "nodeValue"},
> 
> It's not clear to me why this is a pre. It doesn't look like it is in the
> current console, or in Chrome.
Good catch, fixed (using span)

> 
> ::: devtools/client/shared/components/test/mochitest/test_reps_text-node.html
> @@ +48,5 @@
> >        "TextNode rep has expected class names");
> >      is(renderedComponent.textContent, `"hello world"`,
> >        "TextNode rep has expected text content");
> > +
> > +    // Test rendering with EOL character.
> 
> The existing test should be rewritten to cover all of the output. That
> includes the #text part before hand.
The test is improved and checking various modes now, but the '#text' is a title and available only if this.props.objectLink is set. This is the same as e.g. the test for FunctionRep. Perhaps this should be fixed across all reps in another bug?


> Also, why is renderedComponent2 used here? This could be broken up into two
> separate tasks or functions.
Fixed.

Thanks for the review Lin!

Honza

Comment 17

[Lin Clark [:linclark]]

Comment on attachment 8802466 [details] [diff] [review]
bug1281051.patch

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

> but the '#text' is a title and available only if this.props.objectLink is set. This is the same as e.g. the test for FunctionRep. Perhaps this should be fixed across all reps in another bug?

Ah, ok, I didn't realize this. It probably would be good to fix across all the reps, but we can wait to implement that until we are ready to switch console to use the object inspector (instead of variables view) if you like.

LGTM

Comment 18

[Jan Honza Odvarko [:Honza] (always need-info? me)]

(In reply to Lin Clark [:linclark] from comment #17)
> Ah, ok, I didn't realize this. It probably would be good to fix across all
> the reps, but we can wait to implement that until we are ready to switch
> console to use the object inspector (instead of variables view) if you like.
Works for me.

Thanks for the review!

Honza

Comment 19

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/f684580a847f
Fix text-node rep. r=linclark

Comment 20

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/f684580a847f