Closed
Bug 1205563
Opened 9 years ago
Closed 8 years ago
[RTL] devtools-sidebar-alltabs (overflow dropmarker) is placed wrong in RTL locales
Categories
(DevTools :: Inspector, defect, P3)
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)
5.52 KB,
image/png
|
Details | |
1.35 KB,
patch
|
gl
:
review+
|
Details | Diff | Splinter Review |
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.
[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•9 years ago
|
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)
Comment 3•9 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.
Blocks: dt-rtl
Updated•9 years ago
|
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)
Comment 6•9 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•9 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•9 years ago
|
||
Please have a look at my patch and assign me where applicable please, thanks
Attachment #8696572 -
Flags: review?(ntim.bugs)
Updated•9 years ago
|
Assignee: nobody → olasupov
Status: NEW → ASSIGNED
Comment 9•9 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•9 years ago
|
||
Hi Tim, I've changed the attribute value to "11" instead? I hope this suffices.
Attachment #8696819 -
Flags: review?(ntim.bugs)
Comment 11•9 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•9 years ago
|
Attachment #8696819 -
Flags: review?(ntim.bugs)
Assignee | ||
Comment 12•9 years ago
|
||
Patch has been updated.
Attachment #8696572 -
Attachment is obsolete: true
Attachment #8696819 -
Attachment is obsolete: true
Attachment #8696836 -
Flags: review?(ntim.bugs)
Comment 13•9 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 14•9 years ago
|
||
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•9 years ago
|
||
Patch has been updated.
Attachment #8697703 -
Flags: review?(ntim.bugs)
Comment 16•9 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•9 years ago
|
Keywords: checkin-needed
Assignee | ||
Comment 17•9 years ago
|
||
ohh sorry about that, thanks
Comment 18•9 years ago
|
||
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
Comment 19•8 years ago
|
||
Attachment #8696836 -
Attachment is obsolete: true
Attachment #8697703 -
Attachment is obsolete: true
Attachment #8753674 -
Flags: review+
Updated•8 years ago
|
Keywords: checkin-needed
Comment 21•8 years ago
|
||
https://hg.mozilla.org/integration/fx-team/rev/e0fa395e4982a5225e73908a4a0816932043440d Bug 1205563 - Correctly place overflow drop marker for RTL Locales. r=pbro, ntim
Updated•8 years ago
|
Attachment #8753674 -
Attachment is patch: true
Updated•8 years ago
|
Keywords: checkin-needed
Comment 23•8 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/e0fa395e4982
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
status-firefox49:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 49
Updated•6 years ago
|
Product: Firefox → DevTools
You need to log in
before you can comment on or make changes to this bug.
Description
•