891897 - Identity box text padding is wrong in RTL mode

Status: RESOLVED FIXED

(Firefox :: Theme, defect)

Description

[Mike de Boer [:mikedeboer]]

Attachment: Identity Box in RTL mode issue

See screenshot.

The Identity box in RTL mode is not in a good shape; the padding need some love.

Bug 891833 already fixes the background/ border positioning.

Comment 1

[Rakhi(:rakhisharma)]

Hi, I want to work on this bug,will you please assign this to me :).

Comment 2

[:Gijs (he/him)]

(In reply to Rakhi from comment #1)
> Hi, I want to work on this bug,will you please assign this to me :).

Done! Please feel free to ping me for help on IRC or through bugzilla, both with getting set up with a Firefox build environment or with this bug specifically. :-)

Comment 3

[Rakhi(:rakhisharma)]

(In reply to comment #2)

Thanks! Well I have build the setup and succesfully open nightly browser through the link. Will you please help me in finding the page(UX start page) specified in the Attachment of Description ?Everything in my browser looks fine but not looks as shown in attachment. My browser looks normally as normal it is.

Comment 4

[:Gijs (he/him)]

(In reply to Rakhi from comment #3)
> (In reply to comment #2)
> 
> Thanks! Well I have build the setup and succesfully open nightly browser
> through the link. Will you please help me in finding the page(UX start page)
> specified in the Attachment of Description ?Everything in my browser looks
> fine but not looks as shown in attachment. My browser looks normally as
> normal it is.

So "UX" used to be the name of a kind of special build of Firefox that we used to make (this bug was filed a few years back, but is still present). The screenshot is just from the regular "Nightly start page", which is about:home.

In order to get your browser in "right to left" (RTL) mode, normally you'd need to be using a language written right-to-left, like Arabic or something. However, there's a neat add-on you can use instead so you can check how things work using your English Nightly build: https://addons.mozilla.org/en-GB/firefox/addon/force-rtl/

Use that with your nightly build and restart the browser after installing it. If you then open about:home (or about:config, or about:preferences, ...) you should be able to see the problem.

Comment 5

[Rakhi(:rakhisharma)]

Yeah, I am able to see the bug now :-).What I understand from the bug is I need to do some padding changes with that "nightly" so that it looks fine, right?

I was going through the files, I think I need to do changes in this file: browser/themes/shared/identity-block.inc.css, Am I on right path?

I am not able to find the exact class where I need to do changes.Is there is any way to do changes and check hand to hand? 
like we normally do with inspect element, Inspect element is normally usefull for pages(till now what i know),how I check the changes of tabs?

Comment 6

[:Gijs (he/him)]

(In reply to Rakhi from comment #5)
> Yeah, I am able to see the bug now :-).What I understand from the bug is I
> need to do some padding changes with that "nightly" so that it looks fine,
> right?

Great! And yup, that's exactly it.

> I was going through the files, I think I need to do changes in this file:
> browser/themes/shared/identity-block.inc.css, Am I on right path?

Yup!

> I am not able to find the exact class where I need to do changes.Is there is
> any way to do changes and check hand to hand? 

Yes, you can use the browser toolbox: https://developer.mozilla.org/en-US/docs/Tools/Browser_Toolbox

(especially notice that you might need to tweak a setting or two in the regular devtools settings pane for the browser toolbox to show up in the menu - details in that wiki page)

So then, the identity-block file gets build-time included into the platform-specific browser.css files. So in the browser toolbox, you could use the inspector, or if you're using the style editor (which you can use to make live changes to the CSS as well) look for browser.css rather than identity-block.css or something like that.

I just took a quick look, and so a quick note of caution...

As you probably know, padding and margin on the web tend to be specified as "left" or "right", either using padding-left/margin-left, or using the shorthands.

There are "logical" rather than "physical" equivalents, using "start" and "end" in their name - because commonly you want different padding/margin for the different sides, and you'd like the padding/margin to swap around in RTL mode, whereas "left" or "right" remain at the same side.

So you'll find that there's a 'padding-inline-start' already specified in the CSS. That should (should!) do the right thing.

Unfortunately, if you're inspecting this using the devtools, you'll find that the element in question has a style attribute set that changes the CSS direction property of the element. The JS that does this is here: 

https://dxr.mozilla.org/mozilla-central/rev/46210f3ae07855edfe1383210d78433e38a6b564/browser/base/content/browser.js#6872-6874

This is messing things up, because now 'start' is still on the left, even when the browser is using RTL, because that element itself is actually using LTR.

If you were to access a domain with EV certificates that used a RTL language to specify the owner of the domain, and you were using the browser in RTL mode, then it would be laid out correctly, because the element would be set to be in RTL mode, too. However, this also means that you should be able to see the 'wrong' result if you used a browser set to LTR and accessed an EV certified domain with an RTL name -- it's going wrong whenever the direction used in this element and the direction used in the browser don't match. The padding should be based on the browser's locale direction rather than the element's direction.

Final piece of the puzzle: Gecko supports a CSS pseudo-class that lets you select for the browser UI's locale: https://developer.mozilla.org/en-US/docs/Web/CSS/:-moz-locale-dir%28rtl%29  (and the inverse :-moz-locale-dir(ltr) , of course). You can use this to force the padding to be correct for both directions of the UI locale.

Hope that helps!

Final thing before I shut up: once you make a change to the code in your tree, you can rebuild with "./mach build faster" - which will be, well, faster, than rebuilding the entire tree, but will normally rebuild you all the CSS/JS/XUL bits that you'd be changing, so that will make it easier for you to change the bits you need to change and quickly see if it has worked properly.

Comment 7

[Rakhi(:rakhisharma)]

No no no! things messed up.I am done with settings of browser tool but it is not showing any option to do changes with search bars on the top.Just showing inspect element for pages below the search bar :( 

I tried to use style editor there was no file browser.css, so I import that file from my system,but know if I am doing changes with that there is nothing showing live.

does I did something wrong?If not why option for editing tabs are not showing.

I follow the instruction as mention in the mdn page.Apart from that as mention in mdn page after changing the settings page is not asking for any confirmation to disable or anything.as mention in that mdn page they ask confirmation but in mine it doesn't ask.

Comment 8

[:Gijs (he/him)]

(In reply to Rakhi from comment #7)
> No no no! things messed up.I am done with settings of browser tool but it is
> not showing any option to do changes with search bars on the top.Just
> showing inspect element for pages below the search bar :( 
> 
> I tried to use style editor there was no file browser.css, so I import that
> file from my system,but know if I am doing changes with that there is
> nothing showing live.

OK, this sounds like you're still using the regular web/content developer tools that are about the page shown in the browser window.
 
> does I did something wrong?If not why option for editing tabs are not
> showing.
> 
> I follow the instruction as mention in the mdn page.Apart from that as
> mention in mdn page after changing the settings page is not asking for any
> confirmation to disable or anything.as mention in that mdn page they ask
> confirmation but in mine it doesn't ask.

OK, did you try to use the Tools > Web Developer > Browser Toolbox menuitem (or the equivalent "Browser Toolbox" item in the web developer tools toolbar button panel) ?

Comment 9

[Rakhi(:rakhisharma)]

(In reply to :Gijs Kruitbosch from comment #8)
> (In reply to Rakhi from comment #7)
> > No no no! things messed up.I am done with settings of browser tool but it is
> > not showing any option to do changes with search bars on the top.Just
> > showing inspect element for pages below the search bar :( 
> > 
> > I tried to use style editor there was no file browser.css, so I import that
> > file from my system,but know if I am doing changes with that there is
> > nothing showing live.
> 
> OK, this sounds like you're still using the regular web/content developer
> tools that are about the page shown in the browser window.
>  
> > does I did something wrong?If not why option for editing tabs are not
> > showing.
> > 
> > I follow the instruction as mention in the mdn page.Apart from that as
> > mention in mdn page after changing the settings page is not asking for any
> > confirmation to disable or anything.as mention in that mdn page they ask
> > confirmation but in mine it doesn't ask.
> 
> OK, did you try to use the Tools > Web Developer > Browser Toolbox menuitem
> (or the equivalent "Browser Toolbox" item in the web developer tools toolbar
> button panel) ?

Yeah, that's what I was doing wrong.Thanks worked now!

Comment 10

[Rakhi(:rakhisharma)]

Hi Gijs,

I have make the changes to the code in my tree, And rebuild it succesfully now I am able to see the changes in my nightly browser.
Now what should be the step,like how I send it to you?through git? or through mail?

Comment 11

[:Gijs (he/him)]

(In reply to Rakhi from comment #10)
> Hi Gijs,
> 
> I have make the changes to the code in my tree, And rebuild it succesfully
> now I am able to see the changes in my nightly browser.

Great!

> Now what should be the step,like how I send it to you?through git? or
> through mail?

Mozilla is transitioning from using patch files attached to bugs (there's an "add an attachment" link right under the screenshot, at the bottom of the attachments list) to using mozreview.

Either way, you'll want to commit your changes with a message like: "Bug 891897 - make the identity box text padding respect the UI text direction instead of its own text direction, r?gijs" .

I'm not 100% sure from your comment if your tree is using git or mercurial. If you're using mercurial, see this document about mozreview: http://mozilla-version-control-tools.readthedocs.org/en/latest/mozreview/install-mercurial.html#mozreview-install-mercurial , and then it should be enough to just use "hg push -r . review" ( http://mozilla-version-control-tools.readthedocs.org/en/latest/mozreview/commits.html )

I use mercurial rather than git, so I'm not exactly sure how using git for this works... but http://mozilla-version-control-tools.readthedocs.org/en/latest/mozreview/install-git.html should be a reasonable step-by-step guide. You'd end up submitting the commit to mozreview. That should also then show up on the bug. If that's not working for some reason, do ping me here, on IRC or via email.

If you get stuck with that, you could also upload a patch file as an attachment to this bug: https://bugzilla.mozilla.org/attachment.cgi?bugid=891897&action=enter . You should be able to export a patch from hg using "hg export -r . > some-filename.txt", or from git using git format-patch (you might also want https://github.com/mozilla/moz-git-tools/blob/master/git-patch-to-hg-patch -- I'm not sure...).

When you upload the file, make sure to tick the "this is a patch" checkbox near the top of the page. You'll also need to manually request review? from me. There's a list of dropdowns for a number of different flags you can set on an attachment on the attachment page. Set the 'review' flag to '?' and fill in ":gijs" in the textbox that will come up next to it, and you should be good.

Comment 12

[Rakhi(:rakhisharma)]

Hi, I am trying to send the patch using mercurial it says:

rakhi@rakhi:~/mozilla-central$ hg push review
pushing to https://reviewboard-hg.mozilla.org/autoreview
searching for appropriate review repository
redirecting push to https://reviewboard-hg.mozilla.org/gecko
searching for changes
sending [    <=>                                                                                                                        ] 7/2 -7sabort: authorization failed

Have any idea?I searched on google but didn't found anything helpfull.

Comment 13

[:Gijs (he/him)]

(In reply to Rakhi from comment #12)
> Hi, I am trying to send the patch using mercurial it says:
> 
> rakhi@rakhi:~/mozilla-central$ hg push review
> pushing to https://reviewboard-hg.mozilla.org/autoreview
> searching for appropriate review repository
> redirecting push to https://reviewboard-hg.mozilla.org/gecko
> searching for changes
> sending [    <=>                                                            
> ] 7/2 -7sabort: authorization failed
> 
> Have any idea?I searched on google but didn't found anything helpfull.

Does your ~/.hgrc file have a [bugzilla] section with:

username = [e-mail]
apikey = some-api-key-you-created-and-copied-using https://bugzilla.mozilla.org/userprefs.cgi?tab=apikey

?

Also, does "hg push -r . review" work any better?

Comment 14

[Rakhi(:rakhisharma)]

(In reply to :Gijs Kruitbosch from comment #13)
> (In reply to Rakhi from comment #12)
> > Hi, I am trying to send the patch using mercurial it says:
> > 
> > rakhi@rakhi:~/mozilla-central$ hg push review
> > pushing to https://reviewboard-hg.mozilla.org/autoreview
> > searching for appropriate review repository
> > redirecting push to https://reviewboard-hg.mozilla.org/gecko
> > searching for changes
> > sending [    <=>                                                            
> > ] 7/2 -7sabort: authorization failed
> > 
> > Have any idea?I searched on google but didn't found anything helpfull.
> 
> Does your ~/.hgrc file have a [bugzilla] section with:
> 
> username = [e-mail]
> apikey = some-api-key-you-created-and-copied-using
> https://bugzilla.mozilla.org/userprefs.cgi?tab=apikey
> 
> ?
>
yes they have both. 
> Also, does "hg push -r . review" work any better?
Nopes :( giving same output.

Comment 15

[Rakhi(:rakhisharma)]

Hey I tried hg outgoing, I think it worked.Give output like:

rakhi@rakhi:~/mozilla-central$ hg outgoing
comparing with https://hg.mozilla.org/mozilla-central
searching for changes
changeset:   286577:37874dadec62
tag:         tip
user:        Rakhi Sharma <[e-mail]>
date:        Thu Mar 10 18:49:10 2016 +0530
summary:     Browser: themes: Fixed Bug id : 891897

Is all ok here?I am sorry I have just seen the summary but I have included the subjest you specified above like ..r?gijs.. but in next line

Comment 16

[:Gijs (he/him)]

(In reply to Rakhi from comment #15)
> Hey I tried hg outgoing, I think it worked.Give output like:
> 
> rakhi@rakhi:~/mozilla-central$ hg outgoing
> comparing with https://hg.mozilla.org/mozilla-central
> searching for changes
> changeset:   286577:37874dadec62
> tag:         tip
> user:        Rakhi Sharma <[e-mail]>
> date:        Thu Mar 10 18:49:10 2016 +0530
> summary:     Browser: themes: Fixed Bug id : 891897


hg outgoing shows you what commits are different on your machine compared to the remote. So that by itself does not push the commit anywhere else - I still can't see it.

> Is all ok here?I am sorry I have just seen the summary but I have included
> the subjest you specified above like ..r?gijs.. but in next line

No worries. If you use:

hg commit --amend

it will let you change the commit message.

(In reply to Rakhi from comment #14)
> (In reply to :Gijs Kruitbosch from comment #13)
> > username = [e-mail]
> > apikey = some-api-key-you-created-and-copied-using
> > https://bugzilla.mozilla.org/userprefs.cgi?tab=apikey
> > 
> > ?
> >
> yes they have both. 

OK. I just realized, I suspect the docs are missing a step: can you go to: https://reviewboard.mozilla.org/r/ and click "log in" on the top right, then follow the log in flow until you're logged into reviewboard on the web?

Once you're logged in there, try pushing again.

If that still doesn't work: Is the email address listed for "username" exactly the same as "[e-mail]", including uppercase/lowercase (seems on bugzilla you're using uppercase "R")? And are you sure the api key is 100% correct?

Comment 17

[Rakhi(:rakhisharma)]

Attachment: MozReview Request: Bug 891897 - Make the identity box text padding respect the UI text direction instead of its own text direction, r?gijs

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

Comment 18

[Rakhi(:rakhisharma)]

(In reply to :Gijs Kruitbosch from comment #16)
> (In reply to Rakhi from comment #15)
> > Hey I tried hg outgoing, I think it worked.Give output like:
> > 
> > rakhi@rakhi:~/mozilla-central$ hg outgoing
> > comparing with https://hg.mozilla.org/mozilla-central
> > searching for changes
> > changeset:   286577:37874dadec62
> > tag:         tip
> > user:        Rakhi Sharma <[e-mail]>
> > date:        Thu Mar 10 18:49:10 2016 +0530
> > summary:     Browser: themes: Fixed Bug id : 891897
> 
> 
> hg outgoing shows you what commits are different on your machine compared to
> the remote. So that by itself does not push the commit anywhere else - I
> still can't see it.
> 
> > Is all ok here?I am sorry I have just seen the summary but I have included
> > the subjest you specified above like ..r?gijs.. but in next line
> 
> No worries. If you use:
> 
> hg commit --amend
> 
> it will let you change the commit message.
> 
> (In reply to Rakhi from comment #14)
> > (In reply to :Gijs Kruitbosch from comment #13)
> > > username = [e-mail]
> > > apikey = some-api-key-you-created-and-copied-using
> > > https://bugzilla.mozilla.org/userprefs.cgi?tab=apikey
> > > 
> > > ?
> > >
> > yes they have both. 
> 
> OK. I just realized, I suspect the docs are missing a step: can you go to:
> https://reviewboard.mozilla.org/r/ and click "log in" on the top right, then
> follow the log in flow until you're logged into reviewboard on the web?
> 
> Once you're logged in there, try pushing again.

Yeah this worked :)
Now I see the commits here https://reviewboard.mozilla.org/r/ 
Thanks :) Waiting for review now .
  
> If that still doesn't work: Is the email address listed for "username"
> exactly the same as "[e-mail]", including uppercase/lowercase
> (seems on bugzilla you're using uppercase "R")? And are you sure the api key
> is 100% correct?

Comment 19

[:Gijs (he/him)]

Comment on attachment 8729007 [details]
MozReview Request: Bug 891897 - Make the identity box text padding respect the UI text direction instead of its own text direction, r?gijs

https://reviewboard.mozilla.org/r/39219/#review35883

I'm glad mozreview worked out in the end. The summary is perfect, but I'm a little worried about the approach. Can you try my suggestion below, and submit a new patch with this same summary? Thanks!

::: browser/themes/shared/identity-block/identity-block.inc.css:58
(Diff revision 1)
>  #identity-icon-labels {
>    padding-inline-start: 2px;
> +  padding-inline-end: 2px;
>  }

This fixes the issue on RTL, but it also makes the spacing on the other side of the label too big. So in LTR, there is now more space on the left of the | separator that is between the identity label ("Firefox" or "Nightly" or "Some site with EV cert") and the URL itself.

Could you try replacing this CSS rule block with 2 separate ones, one for `#identity-icon-labels:-moz-locale-dir(ltr)` and one for `#identity-icon-labels:-moz-locale-dir(rtl)` ? Or did you do that already and did it not work for some reason?

Comment 20

[Rakhi(:rakhisharma)]

Well I thought of it, but it totally skip from my mind that what I will do here, affect both ltr and rtl. Sorry for that,I am sending a new patch.

Comment 21

[Rakhi(:rakhisharma)]

Attachment: MozReview Request: Bug 891897 - Make the identity box text padding respect the UI text direction instead of its own text direction, r?gijs

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

Comment 22

[Rakhi(:rakhisharma)]

Attachment: MozReview Request: Bug 891897 - Make the identity box text padding respect the UI text direction instead of its own text direction, r?gijs

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

Comment 23

[Rakhi(:rakhisharma)]

(In reply to Rakhi from comment #21)
> Created attachment 8729151 [details]
> MozReview Request: Bug 891897 - Make the identity box text padding respect
> the UI text direction instead of its own text direction, r?gijs
> 
> Review commit: https://reviewboard.mozilla.org/r/39265/diff/#index_header
> See other reviews: https://reviewboard.mozilla.org/r/39265/

Please Skip this,This commit have some whitespace problem which is definately not a good coding style. I am just sending a new patch removing those whitespace.

Comment 24

[:Gijs (he/him)]

Comment on attachment 8729151 [details]
MozReview Request: Bug 891897 - Make the identity box text padding respect the UI text direction instead of its own text direction, r?gijs

https://reviewboard.mozilla.org/r/39265/#review36013

::: browser/themes/shared/identity-block/identity-block.inc.css:59
(Diff revision 1)
> +  padding-inline-start: 2px;
> +  padding-inline-end: 2px;

Hey Rakhi,

This is closer, but it seems this way the remark I made earlier:

(In reply to :Gijs Kruitbosch from comment #19)
> ::: browser/themes/shared/identity-block/identity-block.inc.css:58
> (Diff revision 1)
> >  #identity-icon-labels {
> >    padding-inline-start: 2px;
> > +  padding-inline-end: 2px;
> >  }
> 
> This fixes the issue on RTL, but it also makes the spacing on the other side
> of the label too big. So in LTR, there is now more space on the left of the
> | separator that is between the identity label ("Firefox" or "Nightly" or
> "Some site with EV cert") and the URL itself.

is still true, because you added padding on both sides (where there used to be padding on only one side).

What I meant in comment 6 was that, in these two blocks, you can change only 1 CSS property:

padding-left: 2px;

and

padding-right: 2px;

respectively (you can work out which one goes in which block), so that the padding would be applied on the correct side in both cases. Does that make sense?

When you attach your next patch, can you collapse/squash the patches? You can do this with the hg rebase extension, see https://www.mercurial-scm.org/wiki/RebaseExtension#Collapsing .

Comment 25

[:Gijs (he/him)]

Comment on attachment 8729156 [details]
MozReview Request: Bug 891897 - Make the identity box text padding respect the UI text direction instead of its own text direction, r?gijs

https://reviewboard.mozilla.org/r/39269/#review36015

Comment 26

[:Gijs (he/him)]

https://reviewboard.mozilla.org/r/39269/#review36017

Comment 27

[Rakhi(:rakhisharma)]

(In reply to :Gijs Kruitbosch from comment #24)
> Comment on attachment 8729151 [details]
> MozReview Request: Bug 891897 - Make the identity box text padding respect
> the UI text direction instead of its own text direction, r?gijs
> 
> https://reviewboard.mozilla.org/r/39265/#review36013
> 
> ::: browser/themes/shared/identity-block/identity-block.inc.css:59
> (Diff revision 1)
> > +  padding-inline-start: 2px;
> > +  padding-inline-end: 2px;
> 
> Hey Rakhi,
> 
> This is closer, but it seems this way the remark I made earlier:
> 
> (In reply to :Gijs Kruitbosch from comment #19)
> > ::: browser/themes/shared/identity-block/identity-block.inc.css:58
> > (Diff revision 1)
> > >  #identity-icon-labels {
> > >    padding-inline-start: 2px;
> > > +  padding-inline-end: 2px;
> > >  }
> > 
> > This fixes the issue on RTL, but it also makes the spacing on the other side
> > of the label too big. So in LTR, there is now more space on the left of the
> > | separator that is between the identity label ("Firefox" or "Nightly" or
> > "Some site with EV cert") and the URL itself.
> 
> is still true, because you added padding on both sides (where there used to
> be padding on only one side).
> 
> What I meant in comment 6 was that, in these two blocks, you can change only
> 1 CSS property:
> 
> padding-left: 2px;
> 
> and
> 
> padding-right: 2px;
> 
> respectively (you can work out which one goes in which block), so that the
> padding would be applied on the correct side in both cases. Does that make
> sense?

Hi,Have a query regarding this.I done this beacuse when I remove #identity-icon-labels it  affect both ltr and rtl.So while replacing it with #identity-icon-labels:-moz-locale-dir(ltr)` and one for `#identity-icon-labels:-moz-locale-dir(rtl), I need to take care of both side(start and end). Isn't it? 

> When you attach your next patch, can you collapse/squash the patches? You
> can do this with the hg rebase extension, see
> https://www.mercurial-scm.org/wiki/RebaseExtension#Collapsing .

Comment 28

[:Gijs (he/him)]

(In reply to Rakhi from comment #27)
> Hi,Have a query regarding this.I done this beacuse when I remove
> #identity-icon-labels it  affect both ltr and rtl.So while replacing it with
> #identity-icon-labels:-moz-locale-dir(ltr)` and one for
> `#identity-icon-labels:-moz-locale-dir(rtl), I need to take care of both
> side(start and end). Isn't it? 

Well, in both cases we only want padding 'before' the label, so on the 'start' side. We shouldn't change the padding on the 'end' side (should stay 0). As I noted in comment #6, because the label itself gets a (separate) direction, 'start' and 'end' for the label end up not always putting padding where we'd like there to be padding (namely: left if you're using Firefox in LTR, right if you're using it in RTL). So rather than relying on start and end, I was trying to suggest that we could have two CSS rules. One for the LTR case, and one for the RTL case. The LTR case can set padding-left, and the RTL case can set padding-right. Does that make more sense?

Comment 29

[Rakhi(:rakhisharma)]

Yeah got it.Sorry but when I am trying to rebase my tree but it give error.you can see here http://pastebin.com/GEDRCCe3 .And sorry I am commenting my very small doubts here,I tried to catch you there on IRC but didn't find you online.

Comment 30

[:Gijs (he/him)]

(In reply to Rakhi from comment #29)
> Yeah got it.Sorry but when I am trying to rebase my tree but it give
> error.you can see here http://pastebin.com/GEDRCCe3 .And sorry I am
> commenting my very small doubts here,I tried to catch you there on IRC but
> didn't find you online.

No worries - I was out this morning, sorry about that.

There are a couple of things going on here.

1) initially at least, you had some changes to files that hg is keeping track of, that weren't part of any commits. This was causing a lot of the commands you were trying, to fail. In git terms, this would be something like "unstaged changes", or maybe "staged but not committed" changes (hg doesn't really distinguish between those two things). To see what uncommited changes you have, you can use:

hg status

2) when rebasing and using the '-d' parameter, you would need to give a reference to the 'destination' revision to the '-d' parameter. "E" is presumably from an example. So instead of "E" you'd need to look up a revision hash that you want to use from your tree. A good way to have a look at what commits you have that you've made on top of the commits that you pulled in from mozilla-central is to use "hg wip", which is an alias that "./mach mercurial-setup" can set up for you (maybe you did that already?).

3) when you used "hg up <revision>", you were asked if the merge was OK, and said "no". Now hg has the file marked as having merge conflicts. You should check that the file (which seems to be browser/themes/shared/identity-block/identity-block.inc.css ) is OK by opening it in an editor and checking for any conflict markers (they look like series of ">>>" or "<<<" - but there are not likely to be any). Once you're satisfied that it's fine, you can use:

hg resolve -m browser/themes/shared/identity-block/identity-block.inc.css

to *m*ark the file in question as "resolved". After that using 'hg up' or 'hg rebase' with correct parameters should work again.

I'll try to be online on IRC in a bit - generally, weekends are a bit more difficult because I'm not technically working (much...) during that time, and I might be away from my computer. If I'm not around, you can also ask for help in the #introduction channel on Mozilla's IRC server - there are other people who know how mercurial works who could help you with things like this. :-)


Are you familiar with git and "git rebase --interactive"? If so, perhaps an easier way to do this would be the following:
0) open your ~/.hgrc
1) under the [extensions] header, add a line:
histedit =
2) save the file and exit the editor
3) in your mozilla-central tree, update the local checkout to your last / most recent commit
4) use:
hg histedit -r mozilla-central

and you'll get a file in your editor that looks like git's interactive rebase, and you can use that to fold/squash the changes.

Comment 31

[Rakhi(:rakhisharma)]

Comment on attachment 8729007 [details]
MozReview Request: Bug 891897 - Make the identity box text padding respect the UI text direction instead of its own text direction, r?gijs

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/39219/diff/1-2/

Comment 32

[Rakhi(:rakhisharma)]

Hi, After discussing lot about rebasing and all in irc.I pushed a commit here https://reviewboard.mozilla.org/r/39265/ .It is showing discard.I am not sure why it is hapenning .have you any idea??I have did something worng?

Comment 33

[:Gijs (he/him)]

(In reply to Rakhi(:rakhisharma) from comment #32)
> Hi, After discussing lot about rebasing and all in irc.I pushed a commit
> here https://reviewboard.mozilla.org/r/39265/ .It is showing discard.I am
> not sure why it is hapenning .have you any idea??I have did something worng?

I think you're fine - as best I can tell it's because you initially pushed 1 commit, then replaced that with 3 commits, and then replaced that with 1 commit again. So now for the 2 "extra" ones it shows that they were discarded.

Comment 34

[:Gijs (he/him)]

Comment on attachment 8729007 [details]
MozReview Request: Bug 891897 - Make the identity box text padding respect the UI text direction instead of its own text direction, r?gijs

https://reviewboard.mozilla.org/r/39219/#review36297

Perfect! This looks good to me.

Comment 35

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/0e90c6048f3f

Comment 36

[Rakhi(:rakhisharma)]

Yeah thankyou for review and helping me with my very small and silly confusing doubts :). Looking forward for contributing more.This is going to be great learning experience,learn lot in these days.
Well just got a mail. Here is my mozillian profile https://mozillians.org/en-US/u/Rakhi/ if you want you can vouch me :)

Comment 37

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/0e90c6048f3f