Fix the layout regressions for SI/SL messages introduced in bug 917312

VERIFIED FIXED

Status

Firefox OS
Gaia
VERIFIED FIXED
5 years ago
5 years ago

People

(Reporter: gsvelto, Assigned: gsvelto)

Tracking

unspecified
ARM
Gonk (Firefox OS)

Firefox Tracking Flags

(blocking-b2g:1.3+, b2g18 unaffected, b2g-v1.1hd unaffected, b2g-v1.2 unaffected)

Details

Attachments

(3 attachments, 1 obsolete attachment)

Comment hidden (empty)
(Assignee)

Comment 1

5 years ago
Created attachment 824742 [details]
Screenshot before the bug 917312 landed
(Assignee)

Comment 2

5 years ago
Created attachment 824743 [details]
Screenshot after bug 917312 landed

This screenshot shows the graphic regression caused by bug 917312. It mainly boils down to the font size CSS rules not being applied to the message's text.
(Assignee)

Comment 3

5 years ago
Created attachment 824750 [details] [diff] [review]
[PATCH] Apply the SI/SL message styles correctly

This patch removes the tabs that had been accidentally introduced in index.html and fixes the styles for SI/SL messages restoring the layout seen in attachment 824742 [details].
Assignee: nobody → gsvelto
Status: NEW → ASSIGNED
Attachment #824750 - Flags: review?(felash)
I'm so sorry. My fault :(
(Assignee)

Comment 5

5 years ago
(In reply to José Antonio Olivera Ortega [:jaoo] from comment #4)
> I'm so sorry. My fault :(

Blame the reviewer ;-) I hadn't noticed the small name change in the CSS file and I didn't re-test everything during the final review.
Thanks Gabriele.
Due to https://bugzilla.mozilla.org/show_bug.cgi?id=917312#c82, nominating accordingly.
blocking-b2g: --- → koi?
Since this seems to be an regression.
blocking-b2g: koi? → koi+
Blocks: 917312
No longer depends on: 917312
Comment on attachment 824750 [details] [diff] [review]
[PATCH] Apply the SI/SL message styles correctly

Review of attachment 824750 [details] [diff] [review]:
-----------------------------------------------------------------

r=me with nits
you can ignore the l10n questions and file a non-koi+ bug to fix them.

::: apps/wappush/index.html
@@ +39,5 @@
>  
>    <body role="application">
>      <section role="region">
>        <header>
> +        <button id="close"><span class="icon icon-close"> close </span></button>

no l10n for this ?

@@ +61,5 @@
>        <article id="cp-screen" role="main" hidden>
> +        <section class="container" role="region">
> +          <div class="message">
> +            <p data-l10n-id="cp-container-title"> OTA Access Point Configuration </p>
> +            <input type="hidden" placeholder="PIN" required="">

no l10N for the placeholder?

::: apps/wappush/style/wappush.css
@@ +23,5 @@
>    height: -moz-calc(100% - 5rem - 5rem);
>    overflow: auto;
>  }
>  
> +#si-sl-screen .container .message {

you don't need .container here.
(and neither in the #cp-screen selector)
Attachment #824750 - Flags: review?(felash) → review+
(Assignee)

Comment 9

5 years ago
Created attachment 826768 [details] [diff] [review]
[PATCH v2] Apply the SI/SL message styles correctly r=julienw

Updated patch with all review comments addressed, I've also added the missing L10-strings as we still got time for them :)
Attachment #824750 - Attachment is obsolete: true
(Assignee)

Comment 10

5 years ago
Pushed to gaia/master 4d9741dedd82086f70ea7fd1ee81b1b1c629887c

I won't uplift this to 1.2 unless we uplift bug 917312 too. I'm also not flagging this as affecting 1.2 as the problem is not yet present in that branch.
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
status-b2g18: --- → unaffected
status-b2g-v1.1hd: --- → unaffected
status-b2g-v1.2: --- → unaffected
Resolution: --- → FIXED
Comment on attachment 826768 [details] [diff] [review]
[PATCH v2] Apply the SI/SL message styles correctly r=julienw

Review of attachment 826768 [details] [diff] [review]:
-----------------------------------------------------------------

::: apps/wappush/locales/wappush.en-US.properties
@@ +8,5 @@
>  finish           = Finish
>  
>  this-message-has-expired = This message has expired
>  
> +cp-pin = PIN

oops, it's `cp-pin.placeholder` here
(Assignee)

Comment 12

5 years ago
(In reply to Julien Wajsberg [:julienw] from comment #11)
> oops, it's `cp-pin.placeholder` here

Meh :-/

I've seen both notations in our .properties files though, is this really required or does it work anyway?
"cp-pin.placeholder" sets the "placeholder" property of the node that is pointed by the "cp-pin" data-l10n-id attribute.

Your markup is ok, you only need to fix the locale file.
(Assignee)

Comment 14

5 years ago
(In reply to Julien Wajsberg [:julienw] from comment #13)
> "cp-pin.placeholder" sets the "placeholder" property of the node that is
> pointed by the "cp-pin" data-l10n-id attribute.
> 
> Your markup is ok, you only need to fix the locale file.

OK, I see. I've grepped a bit through our code and it seems we might want to open a follow-up to fix all these kind of occurrences as there's a few of them. For example, these properties files are using '.placeholder' for the 'search-music' string (which is used for a placeholder):

https://github.com/mozilla-b2g/gaia/blob/master/apps/music/locales/music.en-US.properties#L29
https://github.com/mozilla-b2g/gaia/blob/master/apps/music/locales/music.fr.properties#L26

While these are not:

https://github.com/mozilla-b2g/gaia/blob/master/apps/music/locales/music.zh-TW.properties#L25
https://github.com/mozilla-b2g/gaia/blob/master/apps/music/locales/music.ar.properties#L25
The locales that are not the english locales are not important, they should be correct in the gala-l10n repository. We should re-import them from time to time but I think it's not worth fixing them specifically.
(Assignee)

Comment 16

5 years ago
Pushed the placeholder fix to gaia/master 8289bb7633b7154fbff7e27aa3b8bc8525f3433a

Comment 18

5 years ago
Verified on Buri master build.
Gaia:     cc9fec48957c9d217221d403aac4b5f35a826dd6
Gecko:    http://hg.mozilla.org/mozilla-central/rev/16949049f03d
BuildID   20131110040200
Version   28.0a1
Status: RESOLVED → VERIFIED
bug 917312 has been moved to 1.3 - so switching to 1.3+
blocking-b2g: koi+ → 1.3+
You need to log in before you can comment on or make changes to this bug.