Closed Bug 1205563 Opened 4 years ago Closed 4 years ago

[RTL] devtools-sidebar-alltabs (overflow dropmarker) is placed wrong in RTL locales

Categories

(DevTools :: Inspector, defect, P3, trivial)

43 Branch
defect

Tracking

(firefox49 fixed)

RESOLVED FIXED
Firefox 49
Tracking Status
firefox49 --- fixed

People

(Reporter: magicp.jp, Assigned: olasupov, Mentored)

References

(Blocks 2 open bugs)

Details

(Keywords: rtl, Whiteboard: [good first bug][polish-backlog][difficulty=easy][lang=js])

Attachments

(2 files, 4 obsolete files)

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

Steps to reproduce:

1. Run any Firefox version in Arabic.
2. Open Inspector in Developer Tools
3. Display devtools-sidebar-alltabs


Actual results:

devtools-sidebar-alltabs does not have a left border.


Expected results:

devtools-sidebar-alltabs should have a left border, and also does not need a right border.
Blocks: 137995
Component: Untriaged → Developer Tools: Inspector
Keywords: rtl
OS: Unspecified → All
Hardware: Unspecified → All
[right="0"] attribute of .devtools-sidebar-alltabs dropmarker should be replaced with [end="0"]
I haven't read that code, but probably some script creates that dropmarker when you open inspector
So, the attribute in that script should be replaced.
Severity: normal → trivial
Status: UNCONFIRMED → NEW
Ever confirmed: true
Summary: [RTL] devtools-sidebar-alltabs does not have a left border in RTL locales → [RTL] devtools-sidebar-alltabs (overflow dropmarker) is placed wrong in RTL locales
Whiteboard: [good first bug][polish-backlog][difficulty=easy]
Mentor: ntim.bugs
If somebody's going to work on this bug, take into account that the same happens in Netmonitor for .html files. So it's better to file a new report about that/fix it in this bug (I haven't found a dupe)
(In reply to arni2033 from comment #2)
> If somebody's going to work on this bug, take into account that the same
> happens in Netmonitor for .html files. So it's better to file a new report
> about that/fix it in this bug (I haven't found a dupe)

The same code is behind the Inspector and the Netmonitor: https://dxr.mozilla.org/mozilla-central/source/devtools/client/framework/sidebar.js#132
So you can just replace right with end on that file, and both issues are fixed.
Whiteboard: [good first bug][polish-backlog][difficulty=easy] → [good first bug][polish-backlog][difficulty=easy][lang=js]
> Expected results:
>
> devtools-sidebar-alltabs should have a left border, and also does not need a right border.

I'd like to change expected result.

- Keep right border
- devtools-sidebar-alltabs position change from right side (inside) to left side (outside)
Hi I would like to work on this. Please confirm it's OK to proceed.
(In reply to Patrick from comment #5)
> Hi I would like to work on this. Please confirm it's OK to proceed.
Please do. Just post a patch here and we'll assign this bug to you.
Hi I want to have a go at this bug if it's okay, please look at my patch below
Attached patch Bug1205563_sidebar.diff (obsolete) — Splinter Review
Please have a look at my patch and assign me where applicable please, thanks
Attachment #8696572 - Flags: review?(ntim.bugs)
Assignee: nobody → olasupov
Status: NEW → ASSIGNED
Comment on attachment 8696572 [details] [diff] [review]
Bug1205563_sidebar.diff

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

Thanks for the patch ! Looks fine, but why did you set the left attribute to 0 ?
Attachment #8696572 - Flags: review?(ntim.bugs) → feedback+
Attached patch Bug1205563_sidebar.diff (obsolete) — Splinter Review
Hi Tim, I've changed the attribute value to "11" instead? I hope this suffices.
Attachment #8696819 - Flags: review?(ntim.bugs)
(In reply to Victor Olasupo from comment #10)
> Created attachment 8696819 [details] [diff] [review]
> Bug1205563_sidebar.diff
> 
> Hi Tim, I've changed the attribute value to "11" instead? I hope this
> suffices.

I don't understand why we need to set the left attribute at all. Replacing right with end should suffice for this bug.
Attachment #8696819 - Flags: review?(ntim.bugs)
Attached patch Bug1205563_sidebar.diff (obsolete) — Splinter Review
Patch has been updated.
Attachment #8696572 - Attachment is obsolete: true
Attachment #8696819 - Attachment is obsolete: true
Attachment #8696836 - Flags: review?(ntim.bugs)
Comment on attachment 8696836 [details] [diff] [review]
Bug1205563_sidebar.diff

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

Looks good ! This just needs peer review. Also, can you change the commit message to:
Bug 1205563 - Correctly place overflow dropmarker for RTL locales. r=pbro, ntim
Attachment #8696836 - Flags: review?(pbrosset)
Attachment #8696836 - Flags: review?(ntim.bugs)
Attachment #8696836 - Flags: review+
Comment on attachment 8696836 [details] [diff] [review]
Bug1205563_sidebar.diff

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

Looks good to me.
Attachment #8696836 - Flags: review?(pbrosset) → review+
Attached patch Bug1205563_sidebar.diff (obsolete) — Splinter Review
Patch has been updated.
Attachment #8697703 - Flags: review?(ntim.bugs)
Comment on attachment 8697703 [details] [diff] [review]
Bug1205563_sidebar.diff

You don't need to re-request review since :pbro and I already gave you r+
Attachment #8697703 - Flags: review?(ntim.bugs) → review+
Keywords: checkin-needed
ohh sorry about that, thanks
Hi, this patch failed to apply:

renamed 1205563 -> Bug1205563_sidebar.diff
applying Bug1205563_sidebar.diff
patch failed, unable to continue (try -v)
patch failed, rejects left in working directory
errors during apply, please fix and qrefresh Bug1205563_sidebar.diff
Flags: needinfo?(olasupov)
Keywords: checkin-needed
Filter on CLIMBING SHOES
Priority: -- → P3
https://hg.mozilla.org/integration/fx-team/rev/e0fa395e4982a5225e73908a4a0816932043440d
Bug 1205563 - Correctly place overflow drop marker for RTL Locales. r=pbro, ntim
Thanks for the patch Victor. It has been landed!
Flags: needinfo?(olasupov)
https://hg.mozilla.org/mozilla-central/rev/e0fa395e4982
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 49
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.