1333561 - Convert the box model component to use react and redux

Status: RESOLVED FIXED

(DevTools :: Inspector, defect, P3)

Comment 1

[Gabriel Luong [:gl] (ΦωΦ)]

Attachment: Part 1: Add an "old" prefix to the current box model's markup and css. [1.0]

The current plan is to introduce the new rewritten box model into the new layout panel, and have a better box model, grid inspector and z-index inspector ship together when introducing the new layout panel. 

We do want the box model to exist in both computed and layout view, but we are gonna save that for a separate bug. 

In the meantime, we are only gonna show this new box model in the layout view that is hidden behind a pref. We will facilitate this move by keeping the existing the box model until we can switch the pref on and delete the old codebase.

This patch simply prefixes the markup and css styles with "old". So, we can add a new stylesheet for the new box model panel which will have similar html/css structure, but without ids in the html and css.

Comment 2

[Gabriel Luong [:gl] (ΦωΦ)]

Attachment: Part 2: Rename boxmodel.css to oldboxmodel.css. [2.0]

Comment 3

[Gabriel Luong [:gl] (ΦωΦ)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=83b517c0973bedfeab45f936bdb6e3242e8e2b26

Comment 4

[Gabriel Luong [:gl] (ΦωΦ)]

Attachment: Part 1: Add an "old" prefix to the current box model's markup and css. [2.0]

Originally, prefix'd the existing boxmodel.css and renamed the file to oldboxmodel.css in part 2, but instead I have duplicate the boxmodel.css to an oldboxmodel.css and applied the old prefix there. Keeping the boxmodel.css for the new box model panel.

Comment 5

[Gabriel Luong [:gl] (ΦωΦ)]

Attachment: Part 2: Refactor EditingSession to a separate utils module. [1.0]

Comment 6

[Gabriel Luong [:gl] (ΦωΦ)]

Attachment: Part 3: Implements the box model panel in the layout view. [1.0]

You will find a lot of the functionality will be copied over from the original box-model.js. I will add the geometry editor toggle in the next part.

Comment 7

[Gabriel Luong [:gl] (ΦωΦ)]

The layout view is hidden behind the devtools.layoutview.enabled pref.

Comment 8

[Gabriel Luong [:gl] (ΦωΦ)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=a847492e054443f01eaf91317e21ee75408f8502

Comment 9

[Julian Descottes [:jdescottes]]

Comment on attachment 8830146 [details] [diff] [review]
Part 2: Refactor EditingSession to a separate utils module. [1.0]

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

Looks good thanks.

::: devtools/client/inspector/layout/utils/editing-session.js
@@ +61,5 @@
>    getProperty: function (property) {
>      // Create a hidden element for getPropertyFromRule to use
>      let div = this._doc.createElement("div");
>      div.setAttribute("style", "display: none");
>      this._doc.getElementById("sidebar-panel-computedview").appendChild(div);

just a heads up: this is somewhat tied to location of the existing box-model and should be cleaned up if this is supposed to become a shared module used in several panels.

::: devtools/client/inspector/layout/utils/moz.build
@@ +4,5 @@
>  # License, v. 2.0. If a copy of the MPL was not distributed with this
>  # file, You can obtain one at http://mozilla.org/MPL/2.0/.
>  
>  DevToolsModules(
> +    'editing-session.js',

Maybe put this in inspector/shared rather than inspector/layout/utils ?

Depends if you intend the new box model to live in layout/ or in shared/. Since it will be displayed outside of the layout panel I assumed the latter.

Comment 10

[Gabriel Luong [:gl] (ΦωΦ)]

(In reply to Julian Descottes [:jdescottes] from comment #9)
> Comment on attachment 8830146 [details] [diff] [review]
> Part 2: Refactor EditingSession to a separate utils module. [1.0]
> 
> Review of attachment 8830146 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Looks good thanks.
> 
> ::: devtools/client/inspector/layout/utils/editing-session.js
> @@ +61,5 @@
> >    getProperty: function (property) {
> >      // Create a hidden element for getPropertyFromRule to use
> >      let div = this._doc.createElement("div");
> >      div.setAttribute("style", "display: none");
> >      this._doc.getElementById("sidebar-panel-computedview").appendChild(div);
> 
> just a heads up: this is somewhat tied to location of the existing box-model
> and should be cleaned up if this is supposed to become a shared module used
> in several panels.
> 
> ::: devtools/client/inspector/layout/utils/moz.build
> @@ +4,5 @@
> >  # License, v. 2.0. If a copy of the MPL was not distributed with this
> >  # file, You can obtain one at http://mozilla.org/MPL/2.0/.
> >  
> >  DevToolsModules(
> > +    'editing-session.js',
> 
> Maybe put this in inspector/shared rather than inspector/layout/utils ?
> 
> Depends if you intend the new box model to live in layout/ or in shared/.
> Since it will be displayed outside of the layout panel I assumed the latter.

We will eventually move it out of the layout view and have it shared, but I am leaving that for a separate bug since this effort is to only add it into the layout panel for now.

Comment 11

[Julian Descottes [:jdescottes]]

Comment on attachment 8830154 [details] [diff] [review]
Part 3: Implements the box model panel in the layout view. [1.0]

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

The new widget works nicely, thanks!

Some minor issues when testing the widget itself:
- no min width is defined and the boxes start overlapping when resizing the layout panel to a small width
- TAB between editors doesn't work
- font is not monospace

For each one of them, we can move it to dedicated follow-up issues.

My main issue with the current patch is the amount of code duplication with the existing codebase.
I think we could mitigate it in a few spots. Feel free to discuss this. I get the feeling the old box-model
will be around for quite some time and that's why I want to be careful with this. 

Again we don't have to block landing on this, since this is preffed off for now. 
But we should be clear about what we want to do with this, and create blocking issues if we 
aim to dedupe the code.

Regarding the css duplication, at the moment the two stylesheets are nearly identical.

Why do:
> old-selector { ruleset } (in old-boxmodel.css)
> new-selector { same ruleset } (in boxmodel.css)

instead of 
> old-selector, new-selector { ruleset }

Now if the goal of the patch is just to extract a working copy of the widget and then soon after 
completely rework the markup, css etc... then it makes sense to have completely separate files. 
But it should be mentioned as a comment in both boxmodel.css and old-boxmodel.css in this case. 

Keeping the review flag for now, I'd like to have your opinion on my comments.

::: devtools/client/inspector/layout/layout.js
@@ +96,5 @@
> +       * @param  {String} property
> +       *         The name of the property.
> +       */
> +      onShowBoxModelEditor: (element, event, property) => {
> +        let session = new EditingSession({

That's a lot of duplicated code for a patch, both in JS and CSS.
 
I don't know how long we expect both implementations to live together, but we should avoid duplication in the meantime, when possible.

This method, that creates a kind of box-model editor, could be shared between the two implementation?

Also, as mentioned in my other review, EditingSession is creating elements inside of the computed view container. If we use it here already I think this should be fixed.

@@ +323,5 @@
> +    if (!this.reflowFront) {
> +      return;
> +    }
> +
> +    this.reflowFront.off("reflows", this.update);

this.update -> this.updateBoxModel

@@ +333,5 @@
> +   *
> +   * @return  {Promise} that will be resolved when complete.
> +   */
> +  updateBoxModel() {
> +    let lastRequest = Task.spawn((function* () {

Tracking reflows and computing the strings that should be used in the box-model widgets could also be extracted to a common service.

Comment 12

[Gabriel Luong [:gl] (ΦωΦ)]

(In reply to Julian Descottes [:jdescottes] from comment #11)
> Comment on attachment 8830154 [details] [diff] [review]
> Part 3: Implements the box model panel in the layout view. [1.0]
> 
> Review of attachment 8830154 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> The new widget works nicely, thanks!
> 
> Some minor issues when testing the widget itself:
> - no min width is defined and the boxes start overlapping when resizing the
> layout panel to a small width
> - TAB between editors doesn't work
> - font is not monospace
> 
> For each one of them, we can move it to dedicated follow-up issues.
> 
> My main issue with the current patch is the amount of code duplication with
> the existing codebase.
> I think we could mitigate it in a few spots. Feel free to discuss this. I
> get the feeling the old box-model
> will be around for quite some time and that's why I want to be careful with
> this. 
> 
> Again we don't have to block landing on this, since this is preffed off for
> now. 
> But we should be clear about what we want to do with this, and create
> blocking issues if we 
> aim to dedupe the code.
> 
> Regarding the css duplication, at the moment the two stylesheets are nearly
> identical.
> 
> Why do:
> > old-selector { ruleset } (in old-boxmodel.css)
> > new-selector { same ruleset } (in boxmodel.css)
> 
> instead of 
> > old-selector, new-selector { ruleset }
> 
> Now if the goal of the patch is just to extract a working copy of the widget
> and then soon after 
> completely rework the markup, css etc... then it makes sense to have
> completely separate files. 
> But it should be mentioned as a comment in both boxmodel.css and
> old-boxmodel.css in this case. 
> 
> Keeping the review flag for now, I'd like to have your opinion on my
> comments.
> 
> ::: devtools/client/inspector/layout/layout.js
> @@ +96,5 @@
> > +       * @param  {String} property
> > +       *         The name of the property.
> > +       */
> > +      onShowBoxModelEditor: (element, event, property) => {
> > +        let session = new EditingSession({
> 
> That's a lot of duplicated code for a patch, both in JS and CSS.
>  
> I don't know how long we expect both implementations to live together, but
> we should avoid duplication in the meantime, when possible.
> 
> This method, that creates a kind of box-model editor, could be shared
> between the two implementation?
> 
> Also, as mentioned in my other review, EditingSession is creating elements
> inside of the computed view container. If we use it here already I think
> this should be fixed.
> 
> @@ +323,5 @@
> > +    if (!this.reflowFront) {
> > +      return;
> > +    }
> > +
> > +    this.reflowFront.off("reflows", this.update);
> 
> this.update -> this.updateBoxModel
> 
> @@ +333,5 @@
> > +   *
> > +   * @return  {Promise} that will be resolved when complete.
> > +   */
> > +  updateBoxModel() {
> > +    let lastRequest = Task.spawn((function* () {
> 
> Tracking reflows and computing the strings that should be used in the
> box-model widgets could also be extracted to a common service.

Some of the higher level goals:
- Implement the better box model [see META bug]
- The box model component should be usable in both the computed and layout panel
- Better box model should be released with the new layout panel (includes grid, z-index, box model)
- New layout panel releases will feature some of the ux elements you will see in the designs here: https://invis.io/3X87NEBYH#/179720294_Grid. This is the "new" header, and hopefully a link to mdn docs that will help devs learn more about the features in this panel.
- This is ultimately an experiment to see what go faster would be like if we wanted to work a new feature (pref'd) and how do we indicate new features and make it easily learnable.

Next steps:
- Create the redux store at the inspector level so we can have multiple box model in different panels
- Move the box model components to boxmodel/
- Remove the deprecated code

So, the exact plan is to land this react/redux box model pref'd off while having a UCOSP student working on the bugs in the better box model meta/design. My intentions for the existing box-model.js and old-boxmodel.css are to remove them both exactly the moment this layout panel would be pref'd on. We do keep old/new css files because we need a working copy for the better box model effort. Extracting the code duplication would probably not be worth the effort since layout.js is only required if it is pref'd on. We should probably add some warning that these files are deprecated and are there just a placeholder until they can be removed.

I will add some comments to each of the files to indicate the deprecation, but we can also go a bit more aggressive by renaming the file to be box-model-deprecated.js and boxmodel-deprecated.css.

Comment 13

[Gabriel Luong [:gl] (ΦωΦ)]

Comment on attachment 8830154 [details] [diff] [review]
Part 3: Implements the box model panel in the layout view. [1.0]

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

::: devtools/client/inspector/layout/layout.js
@@ +323,5 @@
> +    if (!this.reflowFront) {
> +      return;
> +    }
> +
> +    this.reflowFront.off("reflows", this.update);

Fixed.

@@ +333,5 @@
> +   *
> +   * @return  {Promise} that will be resolved when complete.
> +   */
> +  updateBoxModel() {
> +    let lastRequest = Task.spawn((function* () {

We do need to maintain the layout object that we get in order to add the computed properties view that you see in the better box model designs. This is why the parsing happened in the React component to essentially remove the "px" or give a default value of "-".

Comment 14

[Gabriel Luong [:gl] (ΦωΦ)]

Attachment: Part 1: Add an "old" prefix to the current box model's markup and css. [3.0]

I renamed oldboxmodel.css to deprecated-boxmodel.css and added a comment.

Comment 15

[Gabriel Luong [:gl] (ΦωΦ)]

Attachment: Part 2: Refactor EditingSession to a separate utils module. [2.0]

Now, appends the dummy element to inspector-main-content instead of the computed view

Comment 16

[Gabriel Luong [:gl] (ΦωΦ)]

Attachment: Part 3: Implements the box model panel in the layout view. [2.0]

I would ultimately like to land this asap to unblock the ucosp student from working on the better box model while still continuing to address any issues.

Comment 17

[Gabriel Luong [:gl] (ΦωΦ)]

Attachment: Part 4: Rename box-model.js to deprecated-box-model.js. [1.0]

Comment 18

[Gabriel Luong [:gl] (ΦωΦ)]

Attachment: Part 3: Implements the box model panel in the layout view. [3.0]

Comment 19

[Julian Descottes [:jdescottes]]

(In reply to Gabriel Luong [:gl][1 biz day review guarantee] (ΦωΦ) from comment #16)
> Created attachment 8830610 [details] [diff] [review]
> Part 3: Implements the box model panel in the layout view. [2.0]
> 
> I would ultimately like to land this asap to unblock the ucosp student from
> working on the better box model while still continuing to address any issues.

I don't want to block anybody from working here :( they could import your patches regardless of the fact that they landed in mc.

> My intentions for the existing box-model.js and old-boxmodel.css are to remove them both exactly 
> the moment this layout panel would be pref'd on.

We want to keep a box-model widget in the computed view, right? So it also depends on using the react-based box-model in the computed view panel. I'm just trying to get an idea of how long we'll have to deal with the duplication.

> Extracting the code duplication would probably not be worth the effort since layout.js 
> is only required if it is pref'd on.

My concern is not about people working on the new code, but people fixing bugs on the existing box model. They will most likely realize that the JS/CSS they are modifying is duplicated (when looking for files, for occurrences of strings in the code etc...). 

Let's have a chat today (on vidyo preferably) to discuss this.

Comment 20

[Patrick Brosset <:pbro>]

Drive-by comment: I would also feel more comfortable not duplicating the whole thing here. 
The box-model is being worked on regularly. Maintaining twice the same widget has a cost. Not long ago, someone was working on making it keyboard accessible. So there might be bugs from that which will need to be fixed and tested in 2 places.
How much would it cost to use the new react-based box-model in the computed-view now?
The new one seems to support all the features already, so it sounds like we could do a drop-in replacement (of course there might be some React integration stuff to do, but that doesn't sound too hard, plus we already use React in a bunch of places in the inspector).

Comment 21

[Julian Descottes [:jdescottes]]

Comment on attachment 8830612 [details] [diff] [review]
Part 3: Implements the box model panel in the layout view. [3.0]

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

As discussed, we should remove the old widget as soon as possible to avoid having duplicated code around in the codebase for too long.
Let's make sure we have a follow up for this.

::: devtools/client/inspector/layout/layout.js
@@ +330,5 @@
> +
> +  /**
> +   * Updates the box model panel by dispatching the new layout data.
> +   *
> +   * @return  {Promise} that will be resolved when complete.

return value is not used anywhere

Comment 22

[Julian Descottes [:jdescottes]]

Comment on attachment 8830608 [details] [diff] [review]
Part 1: Add an "old" prefix to the current box model's markup and css. [3.0]

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

I would have preferred the other way around, leaving the old code untouched and creating new different classes for the new react-based widget. 
Beyond the duplication issue already discussed (and which hopefully will go away soon), won't this be an issue if we have to uplift changes?

R+ based on our discussion this week, the old code should go away soon. 
In the future we should start by migrating or removing old widgets rather than duplicating stuff.

Comment 23

[Gabriel Luong [:gl] (ΦωΦ)]

Attachment: Part 3: Implements the box model panel in the layout view. [4.0]

Comment 24

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/ab2849f57624
Part 1: Add an "old" prefix to the current box model's markup and css. r=jdescottes
https://hg.mozilla.org/integration/mozilla-inbound/rev/841363ee81e6
Part 2: Refactor EditingSession to a separate utils module. r=jdescottes
https://hg.mozilla.org/integration/mozilla-inbound/rev/34e6599b9e3c
Part 3: Implements the box model panel in the layout view. r=jdescottes
https://hg.mozilla.org/integration/mozilla-inbound/rev/725a0bf59e2b
Part 4: Rename box-model.js to deprecated-box-model.js. r=jdescottes

Comment 25

[Gabriel Luong [:gl] (ΦωΦ)]

Attachment: Part 5: Replace the computed view tests with the correct box model wrapper id selector. [1.0]

Comment 26

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/e0effb6a369a
Part 5: Replace the computed view tests with the correct box model wrapper id selector. r=me

Comment 27

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

Backed out for failing browser_inspector_textbox-menu.js:

https://hg.mozilla.org/integration/mozilla-inbound/rev/e0effb6a369ade6dc46459c17239f413c397b0ed
https://hg.mozilla.org/integration/mozilla-inbound/rev/725a0bf59e2bb42fa616e7ccc2cab249314d2d3d
https://hg.mozilla.org/integration/mozilla-inbound/rev/34e6599b9e3c8510edf111fa3534b71a693e4403
https://hg.mozilla.org/integration/mozilla-inbound/rev/841363ee81e68afff388cda666e6fa3fc824a04d
https://hg.mozilla.org/integration/mozilla-inbound/rev/ab2849f57624c01c344a55b2632dac1c2603ce4c

Push with failures: https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&revision=e0effb6a369ade6dc46459c17239f413c397b0ed
Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=72829456&repo=mozilla-inbound

[task 2017-01-28T19:15:08.385326Z] 19:15:08     INFO - TEST-PASS | devtools/client/inspector/test/browser_inspector_textbox-menu.js | The menu is closed again - 
[task 2017-01-28T19:15:08.386547Z] 19:15:08     INFO - Switching to the computed-view
[task 2017-01-28T19:15:08.387703Z] 19:15:08     INFO - Testing the box-model region
[task 2017-01-28T19:15:08.388842Z] 19:15:08     INFO - Buffered messages finished
[task 2017-01-28T19:15:08.390042Z] 19:15:08     INFO - TEST-UNEXPECTED-FAIL | devtools/client/inspector/test/browser_inspector_textbox-menu.js | Uncaught exception - at chrome://mochikit/content/tests/SimpleTest/EventUtils.js:458 - TypeError: aTarget is null
[task 2017-01-28T19:15:08.391188Z] 19:15:08     INFO - Stack trace:
[task 2017-01-28T19:15:08.392244Z] 19:15:08     INFO -     synthesizeMouseAtCenter@chrome://mochikit/content/tests/SimpleTest/EventUtils.js:458:7
[task 2017-01-28T19:15:08.393437Z] 19:15:08     INFO -     @chrome://mochitests/content/browser/devtools/client/inspector/test/browser_inspector_textbox-menu.js:69:3

Comment 28

[Pulsebot]

Backout by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/bfc57da36ed8
Backed out changeset e0effb6a369a 
https://hg.mozilla.org/integration/mozilla-inbound/rev/fb98ae7c27b1
Backed out changeset 725a0bf59e2b 
https://hg.mozilla.org/integration/mozilla-inbound/rev/2cb88d039a8b
Backed out changeset 34e6599b9e3c 
https://hg.mozilla.org/integration/mozilla-inbound/rev/2a6f35488b3f
Backed out changeset 841363ee81e6 
https://hg.mozilla.org/integration/mozilla-inbound/rev/5c13e94bb9cf
Backed out changeset ab2849f57624 for failing browser_inspector_textbox-menu.js. r=backout

Comment 29

[Gabriel Luong [:gl] (ΦωΦ)]

Attachment: Part 5: Replace the inspector and computed view tests with the correct box model selectors. [2.0]

Comment 30

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/803fa4177e16
Part 1: Add an "old" prefix to the current box model's markup and css. r=jdescottes
https://hg.mozilla.org/integration/mozilla-inbound/rev/bc1ade7ff7f0
Part 2: Refactor EditingSession to a separate utils module. r=jdescottes
https://hg.mozilla.org/integration/mozilla-inbound/rev/b318e4b93a7b
Part 3: Implements the box model panel in the layout view. r=jdescottes
https://hg.mozilla.org/integration/mozilla-inbound/rev/934733d09f10
Part 4: Rename box-model.js to deprecated-box-model.js. r=jdescottes
https://hg.mozilla.org/integration/mozilla-inbound/rev/f5d5bdc8802c
Part 5: Replace the inspector and computed view tests with the correct box model selectors. r=me

Comment 31

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/803fa4177e16
https://hg.mozilla.org/mozilla-central/rev/bc1ade7ff7f0
https://hg.mozilla.org/mozilla-central/rev/b318e4b93a7b
https://hg.mozilla.org/mozilla-central/rev/934733d09f10
https://hg.mozilla.org/mozilla-central/rev/f5d5bdc8802c

Comment 33

[Cristian Comorasu, QA [:ccomorasu], Releae Desktop QA - Please contact [e-mail]]

Does this issue require manual QA? If so can you please give me some guideline in verifying it? Thank you!

Comment 34

[Gabriel Luong [:gl] (ΦωΦ)]

(In reply to Cristian Comorasu [:CristiComo] from comment #33)
> Does this issue require manual QA? If so can you please give me some
> guideline in verifying it? Thank you!

I don't think issue requires manual QA since we still have our usual test coverage running. Thank you for being on top of this Cristian, I will let you know if anything changes.