Marco Zehe (:MarcoZ) Reporter
	Description • 10 years ago

Otherwise the screen reader won't know what type of control that is.

	Eitan Isaacson [:eeejay]
	Comment 2 • 10 years ago

aurelien,

Did you test this?

	Yura Zenevich [:yzen] (please use "needinfo")
	Comment 3 • 10 years ago

It also most likely need aria-multiline set to true.

	James Burke [:jrburke]
	Comment 4 • 10 years ago

Comment on attachment 8529204 [details] [review]
add role textbox

:yzen's comment around multiline seems relevant. Can you add that, then rebase/squash the pull request so that there is only one commit in the pull request, then flip review back to me.

	aurelien levy
	Comment 5 • 10 years ago

I've done the fix but didn't see how to merge the two commits in the PR (still not very familiar with github)

	aurelien levy
	Comment 6 • 10 years ago

Comment on attachment 8529204 [details] [review]
add role textbox

https://github.com/mozilla-b2g/gaia/pull/26531

	aurelien levy
	Comment 7 • 10 years ago

done but this time it merge my fix with another one I'm working on sorry for that I hope it will be ok

	aurelien levy
	Comment 8 • 10 years ago

Comment on attachment 8529204 [details] [review]
add role textbox

https://github.com/mozilla-b2g/gaia/pull/26531

	Yura Zenevich [:yzen] (please use "needinfo")
	Comment 9 • 10 years ago

(In reply to aurelien levy from comment #7)
> done but this time it merge my fix with another one I'm working on sorry for
> that I hope it will be ok

Hi Aurelien, this is great! We would however need to have a clean pull request in order to be able to include it into master. I would suggest using git cherry-pick to selectively apply only the necessary commit in your branch for this bug. Then you should be able to use git push -f to update the branch/pull request without closing it.

	aurelien levy
	Comment 10 • 10 years ago

Comment on attachment 8529204 [details] [review]
add role textbox

https://github.com/mozilla-b2g/gaia/pull/26535

	aurelien levy
	Comment 11 • 10 years ago

ok this time it must be good I hope

	James Burke [:jrburke]
	Comment 12 • 10 years ago

Comment on attachment 8529204 [details] [review]
add role textbox

This is looking good, but just stuck on some of the mechanics around git. There is definitely a learning curve with git and GitHub, so this is to be expected. Since git is very flexible, this means that there are normally multiple ways to accomplish a task, making it more confusing. Thanks for sticking with it! 

Here is what I suggest to get the change in a format that we expect. This is generally the workflow I use whenever doing a new change. I apologize if this repeats things you already know, just listing it all for completeness.

From your fork of gaia, the one you have at https://github.com/goetsu/gaia, clone that locally, and set up a "remote" that is called "upstream" that points to the mozilla-b2g/gaia repo:

  git clone --recursive git@github.com:goetsu/gaia.git gaia-goetsu
  cd gaia-goetsu
  git remote add upstream git@github.com:mozilla-b2g/gaia.git

For each bug you are working on, create a branch to work on it. This branch will be used for the pull request when you are ready. So taking this bug as an example, and assuming you are starting in your clone's `master` branch in the `gaia-goetsu` directory:

  # this updates your local master to match mozilla-b2g's latest master
  # you should always do this to give better odds your change will work
  # with the lastest master state for when the pull request is merged
  git pull upstream master

  # this updates your fork on github's master to match
  git push origin master
  
  # Create bug-specific branch based on current state of master
  git checkout -b bug1068713-email-compose-role master

Now you will be in the `bug1068713-email-compose-role` branch locally, and its contents will look the same as the `master` branch. The idea with bug-specific branches is that you keep your master branch pristine and only matching what is in the official mozilla-b2g branch. No other local changes. This can be useful for comparisons or rebasing.

Do the changes to apps/email/js/cards/compose.html. As a review note, we are trying to get to a place with our HTML such that no line goes over 80 characters, and this change would push that div over 80. So we just break somewhere in the attribute list. Example:

  <div data-prop="textBodyNode" class="cmp-body-text" 
       contenteditable="true" role="textbox" aria-multiline="true"></div>

Commit the change to the branch and then push the branch to your fork. For the commit message, you can just copy in the title of the bug:

  git commit -am "Bug 1068713 - compose screen: email body should have role textbox"
  git push origin bug1068713-email-compose-role

Now you can go to https://github.com/goetsu/gaia and do the pull request.

In the course of the review, if you need to do other commits to the branch for review feedback, once it is all reviewed, you can flatten all the commits into one commit, then force push the change to your branch. I normally use `rebase -i` for this. So, in the `gaia-goetsu` directory, while you are in the `bug1068713-email-compose-role`, you can run:

  git rebase -i upstream/master

At this point, git gives you a way to edit all the commits. I normally 'pick' the first one, then choose 's' for squash for the rest, so the rest of the commits are squashed to the first picked commit.

Once that is done and git is happy, you can the force push the new state of the branch back to GitHub:

  git push -f origin bug1068713-email-compose-role

---

Once you update this bug with a pull request that gives a result like the above, flip the review back to me, and then it should be easy to r+ it and land.

	aurelien levy
	Comment 13 • 10 years ago

this close to what I have done until the rebase part this is what is missing with the last one ?

	James Burke [:jrburke]
	Comment 14 • 10 years ago

There are two commits in the pull request and from "unknown repository". It should just be one commit, and ideally from a branch/location that github could figure out. I am looking at:

https://github.com/mozilla-b2g/gaia/pull/26494

which is the attachment that was marked for review. It looks like you also left a comment about this pull request:

https://github.com/mozilla-b2g/gaia/pull/26535

That one has 5 commits in it, where ideally there should only be one. It might be good to just start with a fresh branch and do the change (the branch can have any name you like, just helpful if it has bug number in it somewhere).

Then, for whatever pull request you want reviewed, create a new attachment for it. There should be an option (may be hidden at first) when creating the new attachment where you can state that it makes the other attachment "obsolete".

	aurelien levy
	Comment 15 • 10 years ago

yes that the case the only PR to consider is https://github.com/mozilla-b2g/gaia/pull/26535.

So if I understand I need to restart from scratch again to just have one commit in the PR ?
- remove my distant copy on https://github.com/goetsu/gaia
- remove my local copy, 
- refork https://github.com/mozilla-b2g/gaia/ online, 
clone it locally, 
- make the branch locally, 
- make the changes locally, 
- commit the changes to the branch locally, 
- push the branch to online fork, 
- make the PR online
 
Once again sorry about that but I choose the hard way to learn git I suppose ;)

	James Burke [:jrburke]
	Comment 16 • 10 years ago

Your fork of https://github.com/goetsu/gaia is fine, unless you modified your master to be different from mozilla-b2g's master. If that is the case, you can remove your distant copy and starting over as you listed.

We have all had some weird problems with git while learning it and a particular project's process around using it. I still mess up from time to time. The nice thing is that most of it is recoverable by resetting in some way.

	aurelien levy
	Comment 17 • 10 years ago

This time I think it's the good one

	James Burke [:jrburke]
	Comment 18 • 10 years ago

Comment on attachment 8529948 [details] [review]
Bug 1068713 - email body role textbox

Merged in master:
https://github.com/mozilla-b2g/gaia/commit/51e3d63754f3d9e765f811b7174d49bfa3a72d46

from pull request:
https://github.com/mozilla-b2g/gaia/pull/26542

Congratulations, and thank you!

For future pull requests, when modifying the HTML, we are trying to move to a "max 80 characters per line" for them. We do not quite have that yet due to some legacy stuff, but want to do that going forward.