Closed Bug 1004679 Opened 10 years ago Closed 10 years ago

Use text-align: start or end instead of left or right in DevTools themes

Categories

(DevTools :: General, defect)

defect
Not set
normal

Tracking

(firefox34 verified)

VERIFIED FIXED
Firefox 34
Tracking Status
firefox34 --- verified

People

(Reporter: bgrins, Assigned: alexandre-cote, Mentored)

Details

(Whiteboard: [bugday-20140820])

Attachments

(1 file, 2 obsolete files)

There was a patch posted in Bug 552486, which was intended for doing this in SeaMonkey, but actually does it for DevTools.  Patch is here: https://bugzilla.mozilla.org/attachment.cgi?id=8415992&action=edit
Alex, would you mind uploading your patch from Bug 552486 here?  One thing to add about the original patch is that the three separate ruleview.css files have been merged into one at browser/themes/shared/devtools/ruleview.css.
Flags: needinfo?(alexandre-cote)
A search for text align right (a few of these are just for tests and don't matter, and the codemirror.css one should be ignored).

http://dxr.mozilla.org/mozilla-central/search?q=+path%3A*devtools*+%22text-align%3A+right%22&redirect=true
Attachment #8416967 - Flags: review?(bgrinstead)
Flags: needinfo?(alexandre-cote)
(In reply to Brian Grinstead [:bgrins] from comment #1)
> Alex, would you mind uploading your patch from Bug 552486 here?  One thing
> to add about the original patch is that the three separate ruleview.css
> files have been merged into one at
> browser/themes/shared/devtools/ruleview.css.

I have done it.
Whiteboard: [good first bug][mentor=bgrins][lang=css]
Comment on attachment 8416967 [details] [diff] [review]
bug552486_text-align_startorend.diff

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

Thanks for adding the patch.  A couple of notes:

1) There is an error applying this patch on the file: browser/themes/shared/devtools/app-manager/device.css.  Pull down the latest version of the code and update the patch based on this.
2) Please edit your commit message to say: "Bug 1004679 - Use text-align: start or end instead of left or right in DevTools themes; r=bgrins".
Attachment #8416967 - Flags: review?(bgrinstead)
Assignee: nobody → alexandre-cote
Status: NEW → ASSIGNED
Attached patch second try (obsolete) — Splinter Review
Attachment #8416967 - Attachment is obsolete: true
Attachment #8420673 - Flags: review?(bgrinstead)
(In reply to Alex from comment #6)
> Created attachment 8420673 [details] [diff] [review]
> second try

Could it be possible to have a review soon ?
(In reply to Alex from comment #7)
> (In reply to Alex from comment #6)
> > Created attachment 8420673 [details] [diff] [review]
> > second try
> 
> Could it be possible to have a review soon ?

I will take a look at this today.  It may take a little bit to do the review, since I will need to find each change individually and make sure RTL support wasn't relying on the old behavior
Comment on attachment 8420673 [details] [diff] [review]
second try

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

Ryan, there are a couple of changes to app manager CSS here so I wanted to check with you to see if the changes to device.css looked OK.
Attachment #8420673 - Flags: feedback?(jryans)
Comment on attachment 8420673 [details] [diff] [review]
second try

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

Well, the App Manager UI doesn't try very hard to be RTL compatible today...  but anyway, the changes made here seem identical to the current state for both LTR and RTL.  So, seems fine!
Attachment #8420673 - Flags: feedback?(jryans) → feedback+
Comment on attachment 8420673 [details] [diff] [review]
second try

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

OK, this all looks good - but can you update the patch one more time after pulling changes please?  There is an error applying the patch on the latest code:

1 out of 1 hunks FAILED -- saving rejects to file browser/themes/shared/devtools/common.css.rej
Attachment #8420673 - Flags: review?(bgrinstead)
Mentor: bgrinstead
Whiteboard: [good first bug][mentor=bgrins][lang=css] → [good first bug][lang=css]
I am new to bugzilla help me to find how to get assigned to bug and how to resolve it.
(In reply to s.mahavarkar from comment #12)
> I am new to bugzilla help me to find how to get assigned to bug and how to
> resolve it.

Actually, the attached patch is pretty much ready to go, just needs some rebasing.  Please take a look at the bugs marked as good first bugs list: http://mzl.la/1AcYOKF or the list of all mentored bugs: http://mzl.la/1pqY2q8 for something to get started on.
Attached patch text-align.patchSplinter Review
Rebased and pushed to try: https://tbpl.mozilla.org/?tree=Try&rev=02b42a050f7a
Attachment #8420673 - Attachment is obsolete: true
Attachment #8472304 - Flags: review+
Keywords: checkin-needed
https://hg.mozilla.org/integration/fx-team/rev/313e2d2e3df0
Keywords: checkin-needed
Whiteboard: [good first bug][lang=css] → [good first bug][lang=css][fixed-in-fx-team]
https://hg.mozilla.org/mozilla-central/rev/313e2d2e3df0
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Whiteboard: [good first bug][lang=css][fixed-in-fx-team] → [good first bug][lang=css]
Target Milestone: --- → Firefox 34
QA Whiteboard: [good first verify]
Flags: qe-verify+
Tried applying text-align: start and text-align: end on different sites and different tags, seems to work fine.
Status: RESOLVED → VERIFIED
QA Whiteboard: [good first verify] → [bugday-20140820]
Whiteboard: [good first bug][lang=css] → [bugday-20140820]
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.