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

RESOLVED FIXED in Firefox 49

Status

()

Firefox
Developer Tools: Inspector
P3
trivial
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: magicp, Assigned: Victor Olasupo, Mentored)

Tracking

(Blocks: 2 bugs, {rtl})

43 Branch
Firefox 49
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox49 fixed)

Details

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

Attachments

(2 attachments, 4 obsolete attachments)

(Reporter)

Description

2 years ago
Created attachment 8662210 [details]
rtl-devtools-sidebar-alltabs.png

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.
(Reporter)

Updated

2 years ago
Blocks: 137995
Component: Untriaged → Developer Tools: Inspector
Keywords: rtl
OS: Unspecified → All
Hardware: Unspecified → All

Comment 1

2 years ago
[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]

Updated

2 years ago
Mentor: ntim.bugs@gmail.com

Comment 2

2 years ago
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)

Comment 3

2 years ago
(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.

Updated

2 years ago
Whiteboard: [good first bug][polish-backlog][difficulty=easy] → [good first bug][polish-backlog][difficulty=easy][lang=js]
(Reporter)

Comment 4

2 years ago
> 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)

Comment 5

2 years ago
Hi I would like to work on this. Please confirm it's OK to proceed.

Comment 6

2 years ago
(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.
(Assignee)

Comment 7

2 years ago
Hi I want to have a go at this bug if it's okay, please look at my patch below
(Assignee)

Comment 8

2 years ago
Created attachment 8696572 [details] [diff] [review]
Bug1205563_sidebar.diff

Please have a look at my patch and assign me where applicable please, thanks
Attachment #8696572 - Flags: review?(ntim.bugs)

Updated

2 years ago
Assignee: nobody → olasupov
Status: NEW → ASSIGNED

Comment 9

2 years ago
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+
(Assignee)

Comment 10

2 years ago
Created attachment 8696819 [details] [diff] [review]
Bug1205563_sidebar.diff

Hi Tim, I've changed the attribute value to "11" instead? I hope this suffices.
Attachment #8696819 - Flags: review?(ntim.bugs)

Comment 11

2 years ago
(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.

Updated

2 years ago
Attachment #8696819 - Flags: review?(ntim.bugs)
(Assignee)

Comment 12

2 years ago
Created attachment 8696836 [details] [diff] [review]
Bug1205563_sidebar.diff

Patch has been updated.
Attachment #8696572 - Attachment is obsolete: true
Attachment #8696819 - Attachment is obsolete: true
Attachment #8696836 - Flags: review?(ntim.bugs)

Comment 13

2 years ago
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+
(Assignee)

Comment 15

2 years ago
Created attachment 8697703 [details] [diff] [review]
Bug1205563_sidebar.diff

Patch has been updated.
Attachment #8697703 - Flags: review?(ntim.bugs)

Comment 16

2 years ago
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+

Updated

2 years ago
Keywords: checkin-needed
(Assignee)

Comment 17

2 years ago
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
Created attachment 8753674 [details] [diff] [review]
bug_1205563_correctly_place_overflow_drop_marker_for_rtl_locales_r_pbro_nti.patch
Attachment #8696836 - Attachment is obsolete: true
Attachment #8697703 - Attachment is obsolete: true
Attachment #8753674 - Flags: review+

Updated

2 years ago
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)
Attachment #8753674 - Attachment is patch: true
Keywords: checkin-needed

Comment 23

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/e0fa395e4982
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago
status-firefox49: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 49
You need to log in before you can comment on or make changes to this bug.