Closed Bug 932859 Opened 11 years ago Closed 11 years ago

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

Categories

(Firefox OS Graveyard :: Gaia, defect)

Product:
Firefox OS Graveyard
Component:
Gaia
Platform:
ARM
Gonk (Firefox OS)
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

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

Status:
VERIFIED FIXED
Project Flags:
blocking-b2g 1.3+
Tracking Flags:
Tracking Status
b2g18 --- unaffected
b2g-v1.1hd --- unaffected
b2g-v1.2 --- unaffected

People

(Reporter: gsvelto, Assigned: gsvelto)

References

Details

Attachments

(3 files, 1 obsolete file)

Screenshot before the bug 917312 landed
8.51 KB, image/png
Details
Screenshot after bug 917312 landed
6.46 KB, image/png
Details
[PATCH v2] Apply the SI/SL message styles correctly r=julienw
8.67 KB, patch
Details | Diff | Splinter Review 
      No description provided.
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.
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 :(
(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+
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
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
Closed: 11 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
(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.
(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.
Pushed the placeholder fix to gaia/master 8289bb7633b7154fbff7e27aa3b8bc8525f3433a
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.

Attachment

General

Creator:
Created:
Updated:
Size: