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

RESOLVED WONTFIX

Status

defect
RESOLVED WONTFIX
5 years ago
2 years ago

People

(Reporter: eeejay, Unassigned, Mentored, NeedInfo)

Tracking

({access})

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [b2ga11y p=2][mozweekend][lang=css])

Attachments

(3 attachments, 2 obsolete attachments)

This untruth gets read by the screen reader. I think this is a more extensive problem with building blocks using roles for styling (sigh).
Component: Gaia → Gaia::SMS
Depends on: 1055363
Whiteboard: [b2ga11y p=1]
Whiteboard: [b2ga11y p=1] → [b2ga11y p=2]

Comment 3

5 years ago
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.
Attachment #8529179 - Flags: review?(felash)
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

5 years ago
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)
(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 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?
Flags: needinfo?(rnowmrch)
Attachment #8529179 - Flags: review?(felash)
(And Aurélien, I forgot to thank you for your first patch proposal :D)
Mentor: felash
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"] :)
Flags: needinfo?(rnowmrch)

Comment 10

5 years ago
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
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?
Flags: needinfo?(felash)
Flags: needinfo?(eitan)

Comment 13

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

https://github.com/mozilla-b2g/gaia/pull/26531
Attachment #8529179 - Flags: review?(felash)

Comment 14

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

Comment 16

5 years ago
Posted file Bug 1055357 (obsolete) —
This time I hope it's good
Attachment #8529798 - Flags: review?(felash)

Updated

5 years ago
Attachment #8529179 - Attachment is obsolete: true
Attachment #8529179 - Flags: review?(felash)

Updated

5 years ago
Attachment #8529798 - Attachment is obsolete: true
Attachment #8529798 - Flags: review?(felash)

Comment 17

5 years ago
Ok this one is the good one I hope, I commit with both of the changes needed
Attachment #8529950 - Flags: review?(felash)
(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 :)
Flags: needinfo?(felash)
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!
Attachment #8529950 - Flags: review?(felash)
(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.
Flags: needinfo?(eitan)
Summary: Compose message form has a misleading role of "search" → [Messages][Refactoring] Compose message form has a misleading role of "search"
Assigning for Mozilla Weekend Berlin
Assignee: nobody → coder
Hey everyone!
I think I can handle this, let me try
Please do ! but please also read the history in this bug and the previous pull request to understand what's asked here.

Thanks !
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

4 years ago
Hi @Oleksandr, was really nice to meet you! Hopefully your first bug won't evolves into a fundamental discussion. ;)
Attachment #8632555 - Flags: review?(felash)
Whiteboard: [b2ga11y p=2] → [b2ga11y p=2][mozweekend]
Julien, could we please get a review/feedback here? Thanks.
Flags: needinfo?(felash)
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.
Flags: needinfo?(felash)
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 :)
Attachment #8632555 - Flags: review?(felash)
Whiteboard: [b2ga11y p=2][mozweekend] → [b2ga11y p=2][mozweekend][lang=css]
Hey Oleksandr, just wondering if you think you'll be able to come back at this ?
No pressure whatsoever !
Flags: needinfo?(coder)
Flags: needinfo?(rishav006)

Comment 31

4 years ago
Hi Julien, wanted to know if It was ok to submit a patch for this. I can see Oleksandr has not finished this yet.
Flags: needinfo?(felash)
Assigning to nobody as no reply from contributor.
Assignee: coder → nobody
Flags: needinfo?(rishav006)
Hi Patrick,
Feel free to take it :). Comment here if you get any doubt
Flags: needinfo?(felash)
Mass closing of Gaia::SMS bugs. End of an era :(
Status: NEW → RESOLVED
Closed: 2 years ago
Resolution: --- → WONTFIX
Mass closing of Gaia::SMS bugs. End of an era :(
You need to log in before you can comment on or make changes to this bug.