881727 - [leo-pre-iot-br] innecessary truncated tag in Settings > Notifications

Status: RESOLVED FIXED

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

Description

[David Palomino [:dpalomino]]

Device: leo
buildid: 20130529095707 (brazilian variant)

Device is configured in Portuguese. 

In Settings > Notifications, following tag is truncated: 
  - “Exibir c/ tela Bloquea...”

However, the complete sentence would be ““Exibir c/ tela Bloqueado”, so there would be space enough to include the whole sentence.

Comment 1

[Arthur Chen [:arthurcc][inactive after 6/30]]

https://github.com/mozilla-b2g/gaia/blob/master/apps/settings/style/lists.css#L139

The padding-right value should take the size of the control in the list item into consideration. 9rem is perfect for the switch type checkbox, but too large for the normal one.

Comment 2

[Arthur Chen [:arthurcc][inactive after 6/30]]

Attachment: Link to https://github.com/mozilla-b2g/gaia/pull/10420

I added a class to the labels for differentiating the type of the control they embedded. The new padding value 6.1rem came from the difference of the width of a switch (5.1rem) and a checkbox (2.2rem).

Kaze, Pavel, could you help review the patch? Thanks!

Comment 3

[Fabien Cazenave [:kaze]]

If we use a “.switch” class, could we drop the “[data-type=switch]” hack and simplify the related CSS selectors?

(I haven’t really checked whether this is possible or not: it’s an open question)

Comment 4

[Arthur Chen [:arthurcc][inactive after 6/30]]

It is possible to drop "[data-type=switch]". However, "[data-type=switch]" is part of the building block, so that we need to modify all related HTML in whole Gaia while removing it. 

IMHO it would be good if we can tell the type of the controls from the first level element (in this case, <label>), otherwise there is no way to adjust other UI elements accordingly.

Comment 5

[Fabien Cazenave [:kaze]]

(In reply to Arthur Chen [:arthurcc] from comment #4)
> It is possible to drop "[data-type=switch]". However, "[data-type=switch]"
> is part of the building block, so that we need to modify all related HTML in
> whole Gaia while removing it. 

Yes, that’s what I’m talking about. Most of the [data-type=switch] elements are in the Settings app, and there are few occurrences in other apps:

 • 1 in System (#permission-remember-checkbox)
 • 2 in Communications/ftu (#data-connection-switch, #geolocation-switch)
 • 6 in Communications/contacts
 • 6 in CostControl
 • 1 in Geolocation (test_apps) — but I’m not sure it counts

> IMHO it would be good if we can tell the type of the controls from the first
> level element (in this case, <label>), otherwise there is no way to adjust
> other UI elements accordingly.

Exactly. That’s why I’d be in favor to fix the building blocks directly, even if it’s a bit more work.

So far, these building blocks have become more and more complex. We should feel free to improve them by making them simpler, in an iterative process. This looks like a good opportunity to me.

Comment 6

[Arthur Chen [:arthurcc][inactive after 6/30]]

Attachment: Link to https://github.com/mozilla-b2g/gaia/pull/10476

This patch reveals the type information in the top level element. Need some inputs  before sending review requests to app owners, thanks!

Comment 7

[Fabien Cazenave [:kaze]]

Comment on attachment 764565 [details]
Link to https://github.com/mozilla-b2g/gaia/pull/10476

Looks good to me, thanks for improving our building blocks!

Comment 8

[Pavel Ivanov [:ivanovpavel][:pivanov] UX]

looks good to me to :) but you can view my comments on github and improve little more the code :) if you need help :) just ping me

Comment 9

[Arthur Chen [:arthurcc][inactive after 6/30]]

Pavel, I've updated the patch based on your comments. Please take a look at it, thanks!

Comment 10

[Pavel Ivanov [:ivanovpavel][:pivanov] UX]

I put a lot of comments but all of them are for one think on many places. 
we can remove "ul li >" selector from "apps/settings/style/lists.css" like in "shared/style/switches.css" and the other thing is that if really need |label:not([for])| the pr looks good to me :) nice work :)

Comment 11

[Ismael Gonzalez [:basiclines]]

I think this kind of philosophical changes should be done with consistency and armony. Not as a hack to fix an only-one-app related issue.

Comment 12

[Arthur Chen [:arthurcc][inactive after 6/30]]

It looks like we have to decide one of the options:
1. Only add the switch class in settings app without touching building blocks.
2. Apply the changes to only the switch/checkbox/radio controls.
3. Apply the changes to all building blocks. 

Option 3 seems not feasible given the tight schedule. I'm fine with option 1 if we plan to refine all building blocks at one time in the future.

Comment 13

[Dietrich Ayala (:dietrich)]

(In reply to Ismael Gonzalez [:basiclines] from comment #11)
> I think this kind of philosophical changes should be done with consistency
> and armony. Not as a hack to fix an only-one-app related issue.

This is an IOT blocker, holding back many companies and putting the 1.1 release at risk.

We need this fix immediately, even if not visually consistent. Please file a separate bug for the consistency issue.

Comment 14

[Wayne Chang [:wchang]]

Arthur, is the patch ready for review?

Comment 15

[Arthur Chen [:arthurcc][inactive after 6/30]]

The current patch is based on option 2 (comment 12) and ready for review. But we don't have a conclusion on the options yet.

Comment 16

[Arthur Chen [:arthurcc][inactive after 6/30]]

Kaze, Ismael, which option do you prefer?

Comment 17

[Ismael Gonzalez [:basiclines]]

Sorry for taking too long on replying Arthur!
Honestly i will like to go for the option 1.

Comment 18

[Fabien Cazenave [:kaze]]

I’ve recommended to follow option #2 earlier and I still mean it. This building block is broken by design, we should fix it instead of overriding it in a bunch of Settings-specific CSS rules.

More generally, the building blocks have become what they are today because we’ve been piling quick hacks over time:  we’ve had the same 3-option choice over and over, we always chose #1 and never took the time to do #3.

(In reply to Ismael Gonzalez [:basiclines] from comment #11)
> I think this kind of philosophical changes should be done with consistency
> and armony. Not as a hack to fix an only-one-app related issue.

 • “Consistency”: this [data-type=switch] selector is the only data-type we use for switches. and these on/off switches are a very particular case compared to other switches. Changing this block does not affect the overall consistency.

 • “Only-one-app”: this bug affects all apps — Settings being only the most obvious use case because we have a lot of on/off switches.

Now, if we talk about the big picture…

The initial idea was to only rely on standard HTML markup for these building blocks, in order to keep them simple and easy to reuse. This idea became a failure when it’s been decided to handle /all/ UX designs with this kind of markup, and we can see today how difficult it is to fix these blocks.

I think it’s time to move on. Using a data-type attribute rather than a class brings no obvious advantage — especially for this particular switch. I agree we should think of it for the long-term, and as you know I’d like the front-end and UX team to agree a complete taxonomy to define our CSS class names.

There’s been an “un-semantic CSS” subject on m.d.gaia. I’ve suggested to consider other  approaches such as OOCSS in this thread; basically any modular approach that relies on a common (and documented) taxonomy would be fine to me. My suggestion would be to work on that (e.g. by discussing it in this thread before opening a bug) and do incremental changes in our building blocks to make them easier to (re)use.

I think this patch would be a good first step, at least to make sure we’re able to occasionally simplify CSS selectors in our building blocks. ;-)

Comment 19

[Fabien Cazenave [:kaze]]

Ismael, please let me know what you think. If you think we should use another class name to have a better consistency, we’ll follow your suggestion — I expect us to have to refine these BB class names in the future anyway.

If you think the current patch is not acceptable and we should override these BB rules for the Settings app instead, well, so be it: the priority is to fix this bug ASAP. Just let me know.

Honestly, I didn’t expect you to be against a fix for the building blocks. I realize I should have pinged you before telling Arthur to follow option #2. Sorry about that. :-/

Comment 20

[Ismael Gonzalez [:basiclines]]

I'm not against a fix in the building blocks and i know we have to improve them in several ways.
I'm just worried about making such important changes on the selectors because we have a bug that forces us to have a dirty hack in an application.
That is totally legit (if there is an issue, let fix it in the better way possible), but we can't decide a proper classname, prefix taxonomy, etc... without seeing the big picture of the building blocks.

The main problem i see is using unprefixed classes that may collide with other applications because that names are too generic. We should agree in some consistent/bulletproof new taxonomy for the building blocks and i think this is not the proper bug to do it.

So sorry if i've delayed the resolution of this bug, that was not my intention :S but i'm still thinking the best option to take in this case is 1.

Comment 21

[Arthur Chen [:arthurcc][inactive after 6/30]]

Ismael, do you think it is possible to come up a proper class name to avoid conflicts in this bug? If we plan to improve BB iteratly, there must be inconsistent namings in BB before we complete the entire improvement. If we find that we choose an improper class name in the future, replacing it is a lot easier than diving into each app for fixing special overrides. Thus I think maybe we can go for option 2 with a proper class name.

Comment 22

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

We have been using pack- in other BB as prefix. Would make sense using pack-switch, I don't think anybody could be using such a weird name for a class if anyone is worried about class collisions :)

Comment 23

[Ismael Gonzalez [:basiclines]]

That sounds more consistent to me that simply use ".switch" even if we plan to change that prefix in the future, but at least we are not creating new namespaces.

Going for 2 with pack- is ok for me.

Thx guys!

Comment 24

[Arthur Chen [:arthurcc][inactive after 6/30]]

Comment on attachment 764565 [details]
Link to https://github.com/mozilla-b2g/gaia/pull/10476

All, to solve this issue, we need to make the controls reveal their type in the first level element, so that their parent element or sibling element are able to respond to it correctly. 

I removed the "data-type" attribute of the input inside switch, checkbox, radio controls and added corresponding new class names "pack-switch", "pack-checkbox", and "pack-radio" to the label element of the controls. The modification has been approved by the owners of the building blocks.

Based on the modification I searched all three controls in Gaia and made corresponding changes. I separated the patch to several commits, please help review the change, thanks!

Brower: Ben
Calendar: James
Contacts: Alberto
FTU: Fernando
Import: Dale
Cost Control: Salvador
Email: Andrew
Settings: Kaze, Pavel
SMS: Steve
Building blocks: Ismael
System, Test apps: Tim

Comment 25

[Tim Guan-tin Chien [:timdream] (please needinfo)]

Comment on attachment 764565 [details]
Link to https://github.com/mozilla-b2g/gaia/pull/10476

That's a lot! Thank you for doing this.

Comment 26

[Ismael Gonzalez [:basiclines]]

Comment on attachment 764565 [details]
Link to https://github.com/mozilla-b2g/gaia/pull/10476

I've addressed a couple of issues to resolve (easy-fix-ones).
I will give you the r+ now, but plx fix it before merging

Thx

Comment 27

[Steve Chung [:steveck]]

Comment on attachment 764565 [details]
Link to https://github.com/mozilla-b2g/gaia/pull/10476

Looks fine in message app, thanks.

Comment 28

[Arthur Chen [:arthurcc][inactive after 6/30]]

(In reply to Ismael Gonzalez [:basiclines] from comment #26)
> Comment on attachment 764565 [details]
> Link to https://github.com/mozilla-b2g/gaia/pull/10476
> 
> I've addressed a couple of issues to resolve (easy-fix-ones).
> I will give you the r+ now, but plx fix it before merging
> 
> Thx

Ismael, thanks for reviewing. It did miss the list cases and some dynamic added elements. :p

Comment 29

[Arthur Chen [:arthurcc][inactive after 6/30]]

Comment on attachment 764565 [details]
Link to https://github.com/mozilla-b2g/gaia/pull/10476

Etienne, the change also affects call logs. Please help review it, thanks!

Comment 30

[Etienne Segonzac (:etienne)]

Comment on attachment 764565 [details]
Link to https://github.com/mozilla-b2g/gaia/pull/10476

looking good, thanks!

Comment 31

[Salvador de la Puente González [:salva]]

Comment on attachment 764565 [details]
Link to https://github.com/mozilla-b2g/gaia/pull/10476

Tested, no functionality altered.

Thank you.

Comment 32

[James Lal [:lightsofapollo]]

Might want to reassign the review from Alberto not sure if he is still contributing ?

Comment 33

[Arthur Chen [:arthurcc][inactive after 6/30]]

Comment on attachment 764565 [details]
Link to https://github.com/mozilla-b2g/gaia/pull/10476

Per comment 32, reassign the request. Thanks for reminding, James.

Comment 34

[Jose Manuel Cantera]

Comment on attachment 764565 [details]
Link to https://github.com/mozilla-b2g/gaia/pull/10476

r+ for the contacts part

Comment 35

[Fernando Campo (:fcampo)]

Comment on attachment 764565 [details]
Link to https://github.com/mozilla-b2g/gaia/pull/10476

All good for FTU too. Thanks

Comment 36

[Arthur Chen [:arthurcc][inactive after 6/30]]

Hi Ben, are you available to review the simple fix? Or maybe Dale can help on it? Thanks!

Comment 37

[Ben Francis [:benfrancis]]

The attachment already has r+ from Dale, what needs reviewing?

Comment 38

[Dale Harvey (:daleharvey)]

Yup, can double check with ben or just clear the review, I was r?'d to check the cell broadcast I believe, but also checked out the browser

Comment 39

[Arthur Chen [:arthurcc][inactive after 6/30]]

Kaze, Pavel, would you like to review the settings app again or I simply take the feedback+ as r+?

Comment 40

[Arthur Chen [:arthurcc][inactive after 6/30]]

Thanks everyone for reviewing!

master: https://github.com/mozilla-b2g/gaia/commit/6e4ff7535ecad23252fc8360bdc92d27be1c2ee0

Comment 41

[John Ford [:jhford] CET/CEST Berlin Time]

I was not able to uplift this bug to v1-train.  If this bug has dependencies which are not marked in this bug, please comment on this bug.  If this bug depends on patches that aren't approved for v1-train, we need to re-evaluate the approval.  Otherwise, if this is just a merge conflict, you might be able to resolve it with:

  git checkout v1-train
  git cherry-pick -x -m1 6e4ff7535ecad23252fc8360bdc92d27be1c2ee0
  <RESOLVE MERGE CONFLICTS>
  git commit

Comment 42

[Arthur Chen [:arthurcc][inactive after 6/30]]

v1-train: https://github.com/mozilla-b2g/gaia/commit/06d56e0efd1cb3bbed37fef6ee45233b86b3ae3b

Comment 43

[isabelrios]

Attachment: Notifications string

Comment 44

[isabelrios]

Checked with Unagi device V1-train 07/22 build:
Gecko-cf22787
Gaia-41d10fbz

Now the string is not truncated: 'Exibir c/ tela bloqueada' and it appears a checkbox selected by default. 
Please see screenshot attached.

Could anyone please confirm that the string is correct? It is not exactly the same as the one reported in this bug.

Comment 45

[John Ford [:jhford] CET/CEST Berlin Time]

v1.1.0hd: 06d56e0efd1cb3bbed37fef6ee45233b86b3ae3b

Comment 46

[Leo]

This bug causing regression in V1-train Usage app and depends on 897977.