1055357 - [Messages][Refactoring] Compose message form has a misleading role of "search"

Status: RESOLVED WONTFIX

(Firefox OS Graveyard :: Gaia::SMS, defect)

Description

[Eitan Isaacson [:eeejay]]

This untruth gets read by the screen reader. I think this is a more extensive problem with building blocks using roles for styling (sigh).

Comment 1

[aurelien levy]

change made in https://github.com/mozilla-b2g/gaia/pull/26489/commits

Comment 2

[aurelien levy]

css change for this app https://github.com/mozilla-b2g/gaia/pull/26490/commits

Comment 3

[aurelien levy]

Attachment: html and css changes to remove role search of composition form

1ere tentative de pull request je sais pas trop sur quelle branche il faut bosser là là première est sur la 2.1 et la seconde sur master.

Comment 4

[Eitan Isaacson [:eeejay]]

aurelien,

You pull requests are a bit confusing. There should be one pull request for the bug.

Also, this is not only an SMS issue, it is in the shared style sheets, and thus it needs to be removed altogether.

Comment 5

[aurelien levy]

yes I've seen that afterward that why there is two pull requests first one for the html the second one for the style changes (it's first time I create PR for gaia so still no very familiar with the Mozilla conventions on this)

Comment 6

[Julien Wajsberg [:julienw]]

(In reply to Eitan Isaacson [:eeejay] from comment #4)
> 
> Also, this is not only an SMS issue, it is in the shared style sheets, and
> thus it needs to be removed altogether.

Hey Eitan,

From what I see, there are 2 different issues:

1. The SMS app use `role="search"` for its composer input. This is very wrong.
2. We use `role="search"` for styling. This is very wrong too, albeit a little less wrong, for different reasons.

I think that in this bug we should focus on 1), which is what Aurélien tried to do here. Tell me what you think :)



(In reply to aurelien levy from comment #3)
> Created attachment 8529179 [details]
> html and css changes to remove role search of composition form
> 
> 1ere tentative de pull request je sais pas trop sur quelle branche il faut
> bosser là là première est sur la 2.1 et la seconde sur master.

Hey Aurélien :)
First of all, the language we use on Bugzilla is english, so that everybody in the project can read and participate (exception for bugs that discuss about the french locale).

Then we always work on the master branch. Then we possibly cherry-pick the patch on branches (we call this process "uplift"), but I doubt we'll do this here.

So the best is that you do one single pull request for all your changes about this bug.

As commit log, we always use this format as first line:
Bug XXXXX - the purpose of the bug r=reviewer

So in this case it could be:
Bug 1055357 - Compose message form has a misleading role of "search" r=julien

If your pull request's name is in the same format, I think you can add "+shepherd" to the name (one such example is https://github.com/mozilla-b2g/gaia/pull/25868) and a bot should automatically add your pull request to the bug.


I can still have a first look at your changes :)

Comment 7

[Julien Wajsberg [:julienw]]

Comment on attachment 8529179 [details]
html and css changes to remove role search of composition form

I added a comment on the CSS patch. Please merge your changes in one pull request for master only and this should be better.

Also we need to have a plan for the send button. Hey Arnau, what do you think of this? Currently in the SMS app, we use `role="search"` for the composer, but this is a bad idea because obviously it's not a search input. If we remove it, we lose all the nice styling for the button. What do you think of adding a class for this button? We would just add the class to the selectors we already have (so, say, replace
  form[role="search"] button[type="submit"]
by 
  form[role="search"] button[type="submit"], .square-button
).

(I'm not fond of the class name, please suggest another if you find a better idea :) )

What do you think?

Comment 8

[Julien Wajsberg [:julienw]]

(And Aurélien, I forgot to thank you for your first patch proposal :D)

Comment 9

[Arnau March [:arnau] ( not working in Firefox OS anymore :( )]

I would better add the classname replacing form[role="search"]
What do you think about that?

  form[role="search"] button[type="submit"], .input-box button[type="submit"] { ... }

And obviously adding that new classnew everywhere there's a form[role="search"] :)

Comment 10

[aurelien levy]

you mean changing form[role="search"] button[type="submit"] by .input-box button[type="submit"] and so on ? 
If yes I will do that and merge the style pull request with the html fix

Comment 11

[Arnau March [:arnau] ( not working in Firefox OS anymore :( )]

I was suggesting having both options:
form[role="search"] button[type="submit"] and .input-box button[type="submit"]
but It will be better if you replace form[role="search"] with .input-box, and then leave the role when make sense, like in contacts app you could have:
<form role="search" class="input-box" ...>
So you keep the semantic part but you are not use it to style the component.
Feel free to change the classname to anything else like input-wrapper or bb-input-box (so is more specific for building blocks)
Does that work for you guys?

Comment 12

[Mozilla Shepherd (This account is used for automation and doesn't receive bugmail)]

Attachment: mozilla-b2g:master PR#26531

Comment 13

[aurelien levy]

Comment on attachment 8529179 [details]
html and css changes to remove role search of composition form

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

Comment 14

[aurelien levy]

I merge the to pull request in one I hope this is fine

Comment 15

[aurelien levy]

Comment on attachment 8529751 [details] [review]
mozilla-b2g:master PR#26531

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

Comment 16

[aurelien levy]

Attachment: Bug 1055357

This time I hope it's good

Comment 17

[aurelien levy]

Attachment: Bug 1055357 - Compose message form has a misleading role of "search"

Ok this one is the good one I hope, I commit with both of the changes needed

Comment 18

[Julien Wajsberg [:julienw]]

(In reply to Arnau March  [:arnau] from comment #9)
> I would better add the classname replacing form[role="search"]
> What do you think about that?
> 
>   form[role="search"] button[type="submit"], .input-box
> button[type="submit"] { ... }

Actually I really prefer to target the button directly because it makes sense in this case. I mean, I could have such a button even if it's not in an input zone...

But I prefer to move forward and not argue for days, especially that I'm not in the right timezone to have long discussions now :)

Comment 19

[Julien Wajsberg [:julienw]]

Comment on attachment 8529950 [details] [review]
Bug 1055357 - Compose message form has a misleading role of "search"

Hey Aurélien,

sorry, this is not ready, we still miss some styles for the input button. Also, as Arnau suggested, we should seize the opportunity to reduce the technical debt here. Since this is more work than what you likely wanted to do, please tell me if this is too much work and we can decide an alternative path.

More comments on github.

Thanks!

Comment 20

[Eitan Isaacson [:eeejay]]

(In reply to Arnau March  [:arnau] from comment #11)
> I was suggesting having both options:
> form[role="search"] button[type="submit"] and .input-box
> button[type="submit"]
> but It will be better if you replace form[role="search"] with .input-box,
> and then leave the role when make sense, like in contacts app you could have:
> <form role="search" class="input-box" ...>
> So you keep the semantic part but you are not use it to style the component.
> Feel free to change the classname to anything else like input-wrapper or
> bb-input-box (so is more specific for building blocks)
> Does that work for you guys?

Like you said in comment #6, the first goal is to remove role=search in SMS. If we could remove it altogether from BB, that would be a fantastic achievement. Same for any other role selector in BB CSS.

I don't have any specific opinion about what the class/selector should look like.

Comment 21

[Michael Kohler [:mkohler]]

Assigning for Mozilla Weekend Berlin

Comment 22

[Oleksandr Bystrikov]

Hey everyone!
I think I can handle this, let me try

Comment 23

[Julien Wajsberg [:julienw]]

Please do ! but please also read the history in this bug and the previous pull request to understand what's asked here.

Thanks !

Comment 24

[GitHub Autolander Bot]

Attachment: [gaia] softbeehive:master > mozilla-b2g:master

Comment 25

[Oleksandr Bystrikov]

I did manage to fix it the way is probably acceptable. I read carefully previous discussion.
First, take a look at my solution: 
https://github.com/mozilla-b2g/gaia/pull/30929

I assume composer.css needs refactoring, because the recently this trouble of updating single button is caused by inconsistency in css. Small example I found that first :after pseudoelement's content is cleared by selector 
form[role="search"] button[type="submit"]:after {}
and then the same button is hidden by
#messages-send-button:after {}

I don't see any reason to have general selectors like first one if we could use class or id.
Recently I used classes to make sure styles linked to role attribute were not lost. I follow the assumption that this is extending styles.

By the way, I found that composer.css is used in
apps/sms/views/conversation/index.html

Comment 26

[André Fiedler]

Hi @Oleksandr, was really nice to meet you! Hopefully your first bug won't evolves into a fundamental discussion. ;)

Comment 27

[Michael Kohler [:mkohler]]

Julien, could we please get a review/feedback here? Thanks.

Comment 28

[Julien Wajsberg [:julienw]]

Yes, I'm sorry, lately I tried to focus on a big patch I try to land for some weeks. I'll get to this patch shortly.

Comment 29

[Julien Wajsberg [:julienw]]

Comment on attachment 8632555 [details] [review]
[gaia] softbeehive:master > mozilla-b2g:master

Hey Oleksandr !

Sorry again for the delay, as I explained I tried to focus on a very big patch during some days.

I left some comments on github. I think there is some obsolete code here that we can remove (we can tell this is some old part of the app :) ). Thanks for finding the rule that was breaking things :)

I looked very carefully to the CSS changes that come from the removal of role=search, and I found there is a color change for the button -- that you see only in Double SIM mode. But I actually thing the new color is closer to what was expected initially so let's keep it like this.

Please fix the comments I left on github and ask review again once you're ready. Because this is a small patch, you can squash your changes together in one commit (for bigger patches usually we ask that the changes after 1 review are done in a separate commit).

Ah and please don't "merge" master on your pull request, instead please use rebase. In the end there should be only one comment left on your pull request before we merge.

Hope this is clear, please request needinfo if anything is unclear !
Thanks a lot for your work :)

Comment 30

[Julien Wajsberg [:julienw]]

Hey Oleksandr, just wondering if you think you'll be able to come back at this ?
No pressure whatsoever !

Comment 31

[Patrick]

Hi Julien, wanted to know if It was ok to submit a patch for this. I can see Oleksandr has not finished this yet.

Comment 32

[kumar rishav (:rishav_)]

Assigning to nobody as no reply from contributor.

Comment 33

[kumar rishav (:rishav_)]

Hi Patrick,
Feel free to take it :). Comment here if you get any doubt

Comment 34

[Julien Wajsberg [:julienw]]

Mass closing of Gaia::SMS bugs. End of an era :(

Comment 35

[Julien Wajsberg [:julienw]]

Mass closing of Gaia::SMS bugs. End of an era :(