1072805 - The [Cancel] button was misaligned in the IME menu

Status: VERIFIED FIXED

(Firefox OS Graveyard :: Gaia::System::Input Mgmt, defect)

Description

[Rudy Lu [:rudyl] (inactive after 2015/7/3)]

Attachment: ime_button_misaligned.png

STR
===
1. Launch keyboard.
2. Swipe down utility tray and select IME switcher to access the IME menu.
   => See the screenshot for an example.

Comment 1

[Rudy Lu [:rudyl] (inactive after 2015/7/3)]

This was found on current master branch, would like QA's help to see if this would be seen on different branches.

Comment 2

[Pi Wei Cheng [:piwei] (inactive)]

This issue occur on Flame 2.1.

Observed behavior: Keyboard IME menu shows the 'Cancel' button aligned to the right of the screen instead of centered.

Device: Flame
BuildID: 20140925092639
Gaia: 86905e14c3ff06a0e6952ba635b6066ad2eea6b4
Gecko: e128c7d918de
Version: 34.0a2 (2.1)
Firmware: V180
User Agent: Mozilla/5.0 (Mobile; rv:34.0) Gecko/34.0 Firefox/34.0

----------

This issue does NOT occur on Flame 2.0.

Observed behavior: Keyboard IME menu shows the 'Cancel' button aligned in the center.

Device: Flame
BuildID: 20140925082640
Gaia: 95a51689acd0488b3ba79abe2423cdcc132fff4a
Gecko: bd67c37ece85
Version: 32.0 (2.0)
Firmware: V180
User Agent: Mozilla/5.0 (Mobile; rv:32.0) Gecko/32.0 Firefox/32.0

Comment 3

[Joshua Mitchell (Inactive)]

Not nomming, polish issue

Comment 4

[Rudy Lu [:rudyl] (inactive after 2015/7/3)]

Attachment: Patch V1

Patch to add back the styles removed by Bug 1050838.

Alive, Yura, could you please help review this change?
Actually, I don't understand why the patch in Bug 1050838 needs to specify ".value-selector-buttons" for the <menu> element.
So would be good if you could share some thoughts.

Comment 5

[Yura Zenevich [:yzen] (please use "needinfo")]

Comment on attachment 8497469 [details] [review]
Patch V1

Works for me. 

The only 2 nits I have:
* Could we please keep the value-selector related classes, to avoid missing any potential changes to the styling/functionality common to all value selectors.
* Could we move IME specific styling out of value_selector.css somewhere more relevant to IME.

The reason it was added was to keep the markup consistent with the rest of value selectors. Currently the classes are not used for styling, but rather as elements in value_selector. This was done to avoid tying any styling or functionality to specific elements.

Comment 6

[(inactive after 6/18) Alive Kuo [:alive]]

Comment on attachment 8497469 [details] [review]
Patch V1

I don't object to switch to a new class; but it seems the root cause is "full" class doesn't work and we need some different style of ime-menu-container.

Comment 7

[Rudy Lu [:rudyl] (inactive after 2015/7/3)]

(In reply to Yura Zenevich [:yzen] from comment #5)
> Comment on attachment 8497469 [details] [review]
> Patch V1
> 
> Works for me. 
> 
> The only 2 nits I have:
> * Could we please keep the value-selector related classes, to avoid missing
> any potential changes to the styling/functionality common to all value
> selectors.

Those classes that I removed are no longer used anymore, at least for styling part, I cannot find them in shared/ or system CSS files.
So, I just removed them.

> * Could we move IME specific styling out of value_selector.css somewhere
> more relevant to IME.

Done. (I myself forgot we have a separate file for IME menu..)

> 
> The reason it was added was to keep the markup consistent with the rest of
> value selectors. Currently the classes are not used for styling, but rather
> as elements in value_selector. This was done to avoid tying any styling or
> functionality to specific elements.

I think maybe the class names and or DOM hierarchy needs some overhaul after these changes.

Comment 8

[Yura Zenevich [:yzen] (please use "needinfo")]

(In reply to Rudy Lu [:rudyl] from comment #7)
> (In reply to Yura Zenevich [:yzen] from comment #5)
> > Comment on attachment 8497469 [details] [review]
> > Patch V1
> > 
> > Works for me. 
> > 
> > The only 2 nits I have:
> > * Could we please keep the value-selector related classes, to avoid missing
> > any potential changes to the styling/functionality common to all value
> > selectors.
> 
> Those classes that I removed are no longer used anymore, at least for
> styling part, I cannot find them in shared/ or system CSS files.
> So, I just removed them.

You are right, they are not used for styling, but .value-selector-select-options-buttons is definitely used inside value_selector.js:
https://github.com/mozilla-b2g/gaia/blob/master/apps/system/js/value_selector/value_selector.js#L140
https://github.com/mozilla-b2g/gaia/blob/master/apps/system/js/value_selector/value_selector.js#L164

> 
> > * Could we move IME specific styling out of value_selector.css somewhere
> > more relevant to IME.
> 
> Done. (I myself forgot we have a separate file for IME menu..)
> 
> > 
> > The reason it was added was to keep the markup consistent with the rest of
> > value selectors. Currently the classes are not used for styling, but rather
> > as elements in value_selector. This was done to avoid tying any styling or
> > functionality to specific elements.
> 
> I think maybe the class names and or DOM hierarchy needs some overhaul after
> these changes.

Comment 9

[Rudy Lu [:rudyl] (inactive after 2015/7/3)]

(In reply to Yura Zenevich [:yzen] from comment #8)
> (In reply to Rudy Lu [:rudyl] from comment #7)
> > (In reply to Yura Zenevich [:yzen] from comment #5)
> > > Comment on attachment 8497469 [details] [review]
> > > Patch V1
> > > 
> > > Works for me. 
> > > 
> > > The only 2 nits I have:
> > > * Could we please keep the value-selector related classes, to avoid missing
> > > any potential changes to the styling/functionality common to all value
> > > selectors.
> > 
> > Those classes that I removed are no longer used anymore, at least for
> > styling part, I cannot find them in shared/ or system CSS files.
> > So, I just removed them.
> 
> You are right, they are not used for styling, but
> .value-selector-select-options-buttons is definitely used inside
> value_selector.js:
> https://github.com/mozilla-b2g/gaia/blob/master/apps/system/js/
> value_selector/value_selector.js#L140
> https://github.com/mozilla-b2g/gaia/blob/master/apps/system/js/
> value_selector/value_selector.js#L164
> 

Yeah, but the HTML template I modified here will be referenced by "apps/system/js/ime_menu.js" only, and that part of logic would not need ".value-selector-select-options-buttons" for any functionality.

Thanks.

Comment 10

[Rudy Lu [:rudyl] (inactive after 2015/7/3)]

Landed to Gaia master,
https://github.com/mozilla-b2g/gaia/commit/e9fe74a40858944dd38192aef095925af5e8f12b

--
I beleive the Gij failures are irrelevant.

Comment 11

[Rudy Lu [:rudyl] (inactive after 2015/7/3)]

Comment on attachment 8497469 [details] [review]
Patch V1

[Approval Request Comment]
[Bug caused by] (feature/regressing bug #): bug 1050838
[User impact] if declined: This is an obvious UI issue, see attachment 8495080 [details].
[Testing completed]: Yes, manually for UI issue.
[Risk to taking this patch] (and alternatives if risky): Low, CSS changes.
[String changes made]: N/A

Comment 12

[Ryan VanderMeulen [:RyanVM]]

v2.1: https://github.com/mozilla-b2g/gaia/commit/541dc688044a6d20c5a32b86336e508a31e7a188

Comment 13

[Yeojin Chung [:YeojinC]]

This issue is verified fixed on Flame 2.1 and 2.2.

Result: The "Cancel" button is aligned correctly in the IME menu.

Device: Flame 2.1 (319mb)(Kitkat Base)(Full Flash)
BuildID: 20141031001201
Gaia: f89c7b12c36572262c9ea76058694a139b1a8634
Gecko: 50d48f8a04c7
Gonk: 48835395daa6a49b281db62c50805bd6ca24077e
Version: 34.0 (2.1)
Firmware: V188
User Agent: Mozilla/5.0 (Mobile; rv:34.0) Gecko/34.0 Firefox/34.0

Device: Flame 2.2 Master (319mb)(Kitkat Base)(Full Flash)
BuildID: 20141031061804
Gaia: a07994714f0552f89801d6097982308d8b0a1ee1
Gecko: 6bd2071b373f
Gonk: 48835395daa6a49b281db62c50805bd6ca24077e
Version: 36.0a1 (2.2) 
Firmware Version: v188
User Agent: Mozilla/5.0 (Mobile; rv:36.0) Gecko/36.0 Firefox/36.0