Closed Bug 1281051 Opened 6 years ago Closed 5 years ago

Remove text-node rep

Categories

(DevTools :: Shared Components, defect, P1)

defect

Tracking

(firefox50 affected, firefox52 fixed)

RESOLVED FIXED
Firefox 52
Iteration:
52.3 - Nov 14
Tracking Status
firefox50 --- affected
firefox52 --- fixed

People

(Reporter: linclark, Assigned: Honza)

References

Details

(Whiteboard: [reserve-html])

Attachments

(3 files, 3 obsolete files)

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.
Whiteboard: [devtools-html] [triage]
Attached patch Bug1281051.patch (obsolete) — Splinter Review
Attachment #8763682 - Flags: review?(odvarko)
Attached image 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
Attachment #8763815 - Flags: review-
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?
Flags: needinfo?(odvarko)
Assignee: nobody → lclark
Status: NEW → ASSIGNED
Iteration: --- → 50.1
Flags: qe-verify-
Priority: -- → P1
Whiteboard: [devtools-html] [triage] → [devtools-html]
(In reply to Pulsebot from comment #4)
> Pushed by ryanvm@gmail.com:
> https://hg.mozilla.org/integration/fx-team/rev/c4096b7ccbe6
> Remove text-node rep. r=Honza

This was actually bug 1264679.
Attached image 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
Flags: needinfo?(odvarko)
Iteration: 50.1 → 50.2
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 on attachment 8763682 [details] [diff] [review]
Bug1281051.patch

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

Honza
Attachment #8763682 - Flags: review?(odvarko)
Assignee: lclark → nobody
Status: ASSIGNED → NEW
Iteration: 50.2 → ---
Priority: P1 → P2
Priority: P2 → P3
Whiteboard: [devtools-html] → [reserve-html]
Assignee: nobody → odvarko
Status: NEW → ASSIGNED
Iteration: --- → 52.1 - Oct 3
Priority: P3 → P1
(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
Flags: needinfo?(lclark)
For text node, I'd say let's go with whatever Chrome does.
Flags: needinfo?(lclark)
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).
Attached patch bug1281051.patch (obsolete) — Splinter Review
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
Attachment #8763682 - Attachment is obsolete: true
Attachment #8795750 - Flags: review?(lclark)
Iteration: 52.1 - Oct 3 → 52.2 - Oct 17
Should there be a test for this?
Flags: needinfo?(odvarko)
Attached patch bug1281051.patch (obsolete) — Splinter Review
(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
Attachment #8795750 - Attachment is obsolete: true
Attachment #8795750 - Flags: review?(lclark)
Flags: needinfo?(odvarko)
Attachment #8798479 - Flags: review?(lclark)
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.
Attachment #8798479 - Flags: review?(lclark) → review-
Iteration: 52.2 - Oct 17 → 52.3 - Nov 7
Attached patch bug1281051.patchSplinter Review
(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
Attachment #8798479 - Attachment is obsolete: true
Attachment #8802466 - Flags: review?(lclark)
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
Attachment #8802466 - Flags: review?(lclark) → review+
(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
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/f684580a847f
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 52
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.