1224129 - Fix direction of boxmodel margin, size and element size in RTL locales

Status: VERIFIED FIXED

(DevTools :: Inspector, defect, P3)

Description

[magicp]

Attachment: rtl- inspector-element-annotation-and-box-model-header-have-wrong-direction-in-inspector.png

User Agent: Mozilla/5.0 (Windows NT 10.0; WOW64; rv:45.0) Gecko/20100101 Firefox/45.0
Build ID: 20151111030207

Steps to reproduce:

1. Run any versions Firefox in RTL locales (e.g. Arabic)
2. Open any sites (e.g. about:home)
3. Open devtools > Inspector > Box Model
4. Select any elements in HTML pane
5. Confirm inspector-element-annotation and Box Model header


Actual results:

inspector-element-annotation and Box Model header have wrong direction.


Expected results:

inspector-element-annotation and Box Model header should be reverse direction. Please refer to attached image.

Comment 1

[Patrick Brosset <:pbro>]

Triaging (filter on CLIMBING SHOES).

Comment 2

[magicp]

Attachment: revised-expected-results-bug-1224129.png

Revised STR and expected results
> rtl- inspector-element-annotation-and-box-model-header-have-wrong-direction-in-inspector.png
revised-expected-results-bug-1224129.png

> 2. Open any sites (e.g. about:home)
2. Open about:home
> 3. Open devtools > Inspector > Box Model
3. Open DevTools > Inspector > Computed
> 4. Select any elements in HTML pane
4. Select #searchSubmit and #searchSuggestionTable


> Actual results:
> inspector-element-annotation and Box Model header have wrong direction.
boxmodel margin and element size have wrong direction.

> Expected results:
> inspector-element-annotation and Box Model header should be reverse direction. 
.boxmodel-margin, .boxmodel-size and #boxmodel-element-size direction should be LTR.
(.boxmodel-padding and .boxmodel-border are unaffected because cannot set negative value)

Comment 3

[magicp]

FYI, id and class name are different by Firefox version as below.

Firefox 50:
.layout-margin, .layout-size, #layout-element-size

Firefox 51 and later:
.boxmodel-margin, .boxmodel-size, #boxmodel-element-size

Comment 4

[Tomer Cohen :tomer]

Attachment: Testcase

Comment 5

[Tomer Cohen :tomer]

Attachment: Bug 1224129 - Fix direction of boxmodel margin, size and element size in RTL locales

Review commit: https://reviewboard.mozilla.org/r/106168/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/106168/

Comment 6

[Tomer Cohen :tomer]

The patch above fix the following issues:
* content size (width×height)
* border/margin/padding negative values
* box size

Left to do:
* inspection element tooltip is still reversed (height×width)

Comment 7

[Tomer Cohen :tomer]

(In reply to Tomer Cohen :tomer from comment #6)
> * inspection element tooltip is still reversed (height×width)
It seems that it is working well without applying the patch, so this patch is ready. Please let me know if there is yet another location that the box model data is displayed; We have fixed the display of the computed styles panel in bug 1325539.

Comment 8

[Julian Descottes [:jdescottes]]

Attachment: computed-dimension-misplaced.png

Comment 9

[Julian Descottes [:jdescottes]]

Comment on attachment 8828947 [details]
Bug 1224129 - Fix direction of boxmodel margin, size and element size in RTL locales

https://reviewboard.mozilla.org/r/106168/#review107466

Thanks Tomer, this fixes the main problem but I have a small issue with the position of the element-size label (see screenshot attached before this review).

I think we could work around it with the following updates to boxmodel.css:

> #boxmodel-element-size {
>   justify-self: flex-start;
>   direction: ltr;
> }
> 
> #boxmodel-position-group {
>   flex: 1;
>   display: flex;
>   align-items: center;
>   justify-content: flex-end;
> }

Comment 10

[Tomer Cohen :tomer]

Since the boxmodel has been revised by bug 1333561, I think we would deprecate the patch above and start again. 

@jdescottes, what do you suggest? (I'm facing automerge issues with this patch, so this should be r- until a rebase anyway)

Comment 11

[Julian Descottes [:jdescottes]]

It depends how soon we can replace the old boxmodel in favor of the react-based one.
Gabriel: do we have a bug tracking the removal of the deprecated boxmodel?

If both versions are still around for some time then we should probably fix the RTL display issue in both ...

Comment 12

[Julian Descottes [:jdescottes]]

Looks like this is Bug 1336198. Let's block this bug on it.
Tomer, if you'd like to port your patch to the new box-model widget, you can test it in the layout view (to enable in about:config, using devtools.layoutview.enabled)

Comment 13

[Itiel]

magicp, I believe this is now fixed? The boxmodel in Nightly looks very similar to attachment 8806975 [details].

Comment 14

[magicp]

(In reply to Itiel from comment #13)
> magicp, I believe this is now fixed? The boxmodel in Nightly looks very
> similar to attachment 8806975 [details].

Negative number is not fixed. Why did you believe this is now fixed?

Comment 15

[Itiel]

(In reply to magicp from comment #14)
> (In reply to Itiel from comment #13)
> > magicp, I believe this is now fixed? The boxmodel in Nightly looks very
> > similar to attachment 8806975 [details].
> 
> Negative number is not fixed. Why did you believe this is now fixed?

Oh okay, thanks. Didn't notice that.

Comment 16

[Patrick Brosset <:pbro>]

This bug has not been updated in the last 3 months. Resetting the assignee field.
Please, feel free to pick it up again and add a comment outlining your plans for it if you do still intend to work on it.
This is just trying to clean our backlog of bugs and make bugs available for people.

Comment 17

[Itiel]

Attachment: Bug 1224129 - Force LTR on boxmodel items text

Comment 18

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/771185e8e4dc
Force LTR on boxmodel items text r=gl

Comment 19

[Bogdan Tara[:bogdan_tara | bogdant]]

https://hg.mozilla.org/mozilla-central/rev/771185e8e4dc