Closed Bug 986471 Opened 6 years ago Closed 5 years ago

“auto” in box model view should be rotated on sides

Categories

(DevTools :: Inspector, defect)

31 Branch
defect
Not set

Tracking

(Not tracked)

RESOLVED FIXED
Firefox 31

People

(Reporter: nagisa, Assigned: mrspeaker)

Details

(Whiteboard: [good first bug][lang=js][lang=css][mentor=pbrosset])

Attachments

(3 files, 4 obsolete files)

As it says, the “auto” unit should be rotated on left and right in box model view as it currently overflows. See the attached screenshot.
I agree this would help make it more legible.
Going to file this as a good-first-bug because I believe this is quite an easy fix.

I think the changes should go in http://mxr.mozilla.org/mozilla-central/source/browser/devtools/layoutview/view.js#326
What I can think of quickly off the top of my head is checking the length of whatever text value we're displaying in the box model regions, and if the length is more than, say, 4 characters, then add a `rotate` class to the element, and with css, simply rotate elements that have that class.
Status: UNCONFIRMED → NEW
Ever confirmed: true
OS: Linux → All
Hardware: x86_64 → All
Whiteboard: [good first bug][lang=js][lang=css][mentor=pbrosset]
Attached patch rotateLongLables.patch (obsolete) — Splinter Review
I made a patch that works Patrick described (though 4 characters will overlap, so I made the max length 3 before rotating... if this is unacceptable then I think a better solutions would be to measure the text).

Long labels on the left get rotated counter-clockwise and labels on the right get rotated clockwise. I'll attach an image to show it in action.
Attachment #8409082 - Flags: review?(pbrosset)
Attached image Rotated labels in box model (obsolete) —
Attached patch Rotate long labels (obsolete) — Splinter Review
I asked on the #introduction irc channel about my patch format (because I'd created the patch in git) and they pointed out some errors. Hopefully fixed.
Attachment #8409756 - Flags: review?(pbrosset)
Attachment #8409082 - Attachment is obsolete: true
Attachment #8409082 - Flags: review?(pbrosset)
Attachment #8409756 - Attachment description: Fix the HG patch header → Rotate long labels
Comment on attachment 8409756 [details] [diff] [review]
Rotate long labels

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

Thanks for the patch!
The screenshot looks great.
I've applied the patch, it seems to work perfectly.

I made a few comments below.

I'll happily R+ this patch, but I think we also should add a test for this feature, to avoid future regressions.
See these links:
- https://wiki.mozilla.org/DevTools/Hacking#DevTools_Mochitests
- https://wiki.mozilla.org/DevTools/mochitests_coding_standards
The layout-view already has a bunch of tests in /browser/devtools/layoutview/test/
By following how other tests are done, I think it would be easy enough to add a new one that loads a simple test page with, to start with, short labels and then long labels, every time checking if the container for these labels has been rotated or not.

::: browser/devtools/layoutview/view.css
@@ +171,5 @@
>  }
>  .margin.left {
>    left: 0;
>  }
> +.rotate.left {

nit: missing empty line before this selector. We normally have a convention where all rules are spaced by one empty line.

::: browser/devtools/layoutview/view.js
@@ +163,5 @@
>        marginLeft: {selector: ".margin.left > span",
>                    property: "margin-left",
>                    realProperty: "margin-left-value",
> +                  value: undefined,
> +                  rotateClass: "left"},

I think something like `overflowClass` would describe better what this is about.
Also, defining `left` and `right` here is a little odd because these classes aren't actually added to the element, because they're already there, you're just testing if the this property exists.

So perhaps, it should instead be something like:
`rotateOnOverflow: true`

and down below:
`if (this.map[i].rotateOnOverflow) {...`

@@ +393,5 @@
>            continue;
>          }
>          span.textContent = this.map[i].value;
> +
> +        // Rotate label container 90 degrees if too long

Based on my earlier comment, this would need to be rewritten a little bit.
Also, could you extract this part in another function? The update function is already more than 80 lines long, and getting harder to read with every new addition.
It would make it easier to read if you just called a function `manageOverflowingText` here, or something like that.

@@ +395,5 @@
>          span.textContent = this.map[i].value;
> +
> +        // Rotate label container 90 degrees if too long
> +        if (this.map[i].rotateClass) {
> +          if (span.textContent.length > 3) {

3 is a magic number here, can you define something like `const LONG_TEXT_ROTATE_LIMIT = 3;` at the top of the file and use the const here instead.
Attachment #8409756 - Flags: review?(pbrosset)
Excellent feedback, thanks! I'll add the tests, and refactor the code as you suggested. I agree that "overflowClass" is better - I spent a bit of time thinking about that one... naming stuff is hard.
(In reply to Earle Castledine from comment #6)
> naming stuff is hard.
Yeah!
After moving the logic into a function I realized it could be simplified further - just checking for the existence of ".left" or ".right" classes to test if a label can be rotated.

I wrote a test for it that will test if long labels (the all the "auto" fields in the current tests) rotate correctly... but it seems that the layoutview tests are not being run at all. In the devtools/layoutview/browser.ini there is a line `skip-if = true` - that seems to prevent the browser_layoutview.js tests from executing. I removed this to create my test, but I did include that in my patch because I don't understand it.
Attachment #8409083 - Attachment is obsolete: true
Attachment #8409756 - Attachment is obsolete: true
Attachment #8410527 - Flags: review?(pbrosset)
(In reply to Earle Castledine from comment #8)
> Created attachment 8410527 [details] [diff] [review]
> rotate long labels in box model - refactored, plus test
> 
> After moving the logic into a function I realized it could be simplified
> further - just checking for the existence of ".left" or ".right" classes to
> test if a label can be rotated.
> 
> I wrote a test for it that will test if long labels (the all the "auto"
> fields in the current tests) rotate correctly... but it seems that the
> layoutview tests are not being run at all. In the
> devtools/layoutview/browser.ini there is a line `skip-if = true` - that
> seems to prevent the browser_layoutview.js tests from executing. I removed
> this to create my test, but I did include that in my patch because I don't
> understand it.
That's right, I forgot about that ... It's because the layout-view currently doesn't use the right types of events to refresh itself. It uses mozAfterPaint events locally which causes the tests to be really flaky and fail intermittently. So we've decided to disable them for a while.
I'm working on using reflow events instead, so hopefully in a week or so, if I manage to land my changes, we can re-enable the tests.
For your bug, I guess having your test pass locally by removing the skip-if line is enough. Then, as you said, let's just keep this skip line in for now and we'll make sure the test passes on our automated test platforms when we remove the skip.

Going to review your patch a bit later today.
Comment on attachment 8410527 [details] [diff] [review]
rotate long labels in box model - refactored, plus test

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

Thanks for refactoring this. The code is simpler this way \o/
I've made a couple of minor comments below.
Overall, I'm ok to land this as it is.

However, I just realized something a little bit annoying that I think we should fix before.

When you click on one of the values, it becomes editable, and as you type, the field might be rotated suddenly. I think that's because we do live preview of the changes, so when you type, the value is sent, then interpreted and applied, and as a result, the layout-view is updated too, while the field is still visible.
So the field's parent node may have its `rotate` class toggled at any time.
It can make it a little bit harder to use.
Also, if a value is already rotated, the editor that appears when clicking it will be rotated too.

I think the right way to fix this is to add a class or attribute to the parentNode (containing both the label and the editor) while the value is being edited, and use this class to avoid transforming the element.

Here is an example of code that seems to work:

In the `initEditor` function:

    let editor = new InplaceEditor({
      element: element,
      initial: initialValue,

      start: (editor) => {
        editor.elt.parentNode.classList.add("editing");
      },

      change: (value) => {
        if (NUMERIC.test(value))
          value += "px";
        let properties = [
          { name: property, value: value }
        ]

        if (property.substring(0, 7) == "border-") {
          let bprop = property.substring(0, property.length - 5) + "style";
          let style = session.getProperty(bprop);
          if (!style || style == "none" || style == "hidden")
            properties.push({ name: bprop, value: "solid" });
        }

        session.setProperties(properties);
      },

      done: (value, commit) => {
        editor.elt.parentNode.classList.remove("editing");
        if (!commit)
          session.revert();
      }
    }, event);

As you can see I just defined a `start` callback to add a class. And I used the existing `done` callback to remove that class.

Then, in view.css:

.rotate.left:not(.editing) {
  transform: rotate(-90deg);
}

.rotate.right:not(.editing) {
  transform: rotate(90deg);
}

Sorry for adding this last minute change, but I think landing the patch without this may seem like a regression to people using the editor field.

::: browser/devtools/layoutview/test/browser_layoutview.js
@@ +95,5 @@
>      is(elt.textContent, res2[i].value, res2[i].selector + " has the right value after style update.");
>    }
>  });
> +
> +addTest("Test that long labels on left/right are rotated 90 degrees",

I normally would have advised to create a new test file for this instead of adding to this existing test. 
That's because we like to keep our tests short:
- so they're easier to maintain and debug
- so they're less chances that they take too long to run on slow test machines and eventually time out.
But since the tests are all disabled for now and since I plan on cleaning up these tests a bit anyway, no need to do this now.

@@ +98,5 @@
> +
> +addTest("Test that long labels on left/right are rotated 90 degrees",
> +function*() {
> +  let viewdoc = view.document;
> +  const LONG_TEXT_ROTATE_LIMIT = 3;

It's a little annoying that we need to re-define this const here, cause it means if we change it in the code, there's a chance we'll forget to change it in the test or vice versa.
It's not a big deal either because the test will probably fail, giving us a chance to correct it.
The other possibility is to define LONG_TEXT_ROTATE_LIMIT as a property on LayoutView.prototype, this way, making it reachable from the test. But I haven't seen this in a lot of other places of our code, so I'd keep it the way you've done it for now.
Attachment #8410527 - Flags: review?(pbrosset)
Great feedback again. Good idea for the "rotate while editing" issue - I'll add it today (I did notice this issue when I was testing, but I kinda liked it!).

For the const re-definition... I was thinking that would be ok because for the tests it might make sense to test rotating at different lengths.

If you get back here before I land the patch could you assign this bug to me? Thanks again for the feedback - this has been a great learning experience.
And thank *you* for the patch.
Assigning the bug to you now.
I'm ok with the const in the test.
Assignee: nobody → mrspeaker
Added in your fix for the "don't rotate when editing". You're right: that feels much nicer now.
Attachment #8410527 - Attachment is obsolete: true
Attachment #8411183 - Flags: review?(pbrosset)
Comment on attachment 8411183 [details] [diff] [review]
rotate long labels in box model - no rotate on edit.

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

Looks good to me!
I've pushed this patch to TRY: https://tbpl.mozilla.org/?tree=Try&rev=4136911f80f2
I don't see a reason why this should come back with failures since the layout-view tests are disabled anyway, but just in case.
Let's wait until this is done and land the patch then.
Attachment #8411183 - Flags: review?(pbrosset) → review+
Oh, one more thing, we need to add "; r=pbrosset" at the end of the commit description.
Cool! But I'm not sure about this step: I guess it means you reviewed the patch, right? I've seen these in commit messages but I they must have been added automagically. 

When during the process should have I added that comment? I mean, is this the normal procedure for patches: patch -> review -> fix patch -> review ok'd -> amend commit message for reviewer -> done?
Yeah, adding "r=someone" in the commit message is used to know who reviewed any given changeset in the tree.
It's not added automatically, you normally add it, once you know who's going to review the patch, or after it's been reviewed. There's no process for this, but it's not a big deal because commit messages can be amended pretty easily.

Using HG, what I normally do is:

> hg qnew bug123456-something-Im-working-on.patch
This creates a new patch in my hg queue to work on bug123456.
This also starts my text editor to enter the commit message. There I usually write something like this:
> Bug 123456 - What will be fixed; r=someone
At least if I know <someone> is going to review the patch, otherwise, I skip that last part.

> hg qref
During my work on the patch, I regularly refresh the patch with my local changes

> hg qref -m "Bug 123456 - What will be fixed; r=someone"
If I want to change the commit message, once I know who will review the patch for instance.

More information here: 
- https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide
- https://developer.mozilla.org/en-US/docs/Developer_Guide/How_to_Submit_a_Patch
- https://developer.mozilla.org/en-US/docs/Mercurial

If you're using Git, then it's a little bit different. There's some information here: https://developer.mozilla.org/en/docs/Git and here: https://etherpad.mozilla.org/moz-git-tools
But essentially, you'll need to produce a single, rebased, commit at the end with the commit message being something like "Bug 123456 - What will be fixed; r=someone".
I used to use git to work on Mozilla in the past, but I always was exporting patches from git and into my HG queue to finalize them there.

In any case, the try build is green, I'll amend the patch and land it in fx-team now! Thanks.
Fixed in fx-team: https://hg.mozilla.org/integration/fx-team/rev/07aed296db2a
Whiteboard: [good first bug][lang=js][lang=css][mentor=pbrosset] → [fixed-in-fx-team][good first bug][lang=js][lang=css][mentor=pbrosset]
Status: NEW → ASSIGNED
Awesome! I think I'll just start learning Mercurial as that seems to be the "correct" way to do things.  Thanks again for all your help.
https://hg.mozilla.org/mozilla-central/rev/07aed296db2a
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team][good first bug][lang=js][lang=css][mentor=pbrosset] → [good first bug][lang=js][lang=css][mentor=pbrosset]
Target Milestone: --- → Firefox 31
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.