[cros] Error bubble on login is displayed to the left of Gaia frame.

[cros] Error bubble on login is displayed to the left of Gaia frame.

*) Error bubble supports multiple attachments.
*) Delayed bubble display until the iframe is loaded.


BUG=chromium-os:23296, chromium-os:21246
TEST=Manual: see bug description

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=120854

[Ivan Korotkov, 2012-02-02 09:14:58]

PTAL

[xiyuan, 2012-02-02 22:43:52]

http://codereview.chromium.org/9310050/diff/1/chrome/browser/resources/chrome...
File chrome/browser/resources/chromeos/login/bubble.css (right):

http://codereview.chromium.org/9310050/diff/1/chrome/browser/resources/chrome...
chrome/browser/resources/chromeos/login/bubble.css:22: .bubble-right
nit: insert a blank line between css groups and all other similar places.

http://codereview.chromium.org/9310050/diff/1/chrome/browser/resources/chrome...
File chrome/browser/resources/chromeos/login/bubble.js (right):

http://codereview.chromium.org/9310050/diff/1/chrome/browser/resources/chrome...
chrome/browser/resources/chromeos/login/bubble.js:116: pos.bottom =
origin.bottom + el.offsetHeight + PADDING;
Should this be
pos.bottom = origin.top - PADDING
to make bubble sit on top of the anchor element?

http://codereview.chromium.org/9310050/diff/1/chrome/browser/resources/chrome...
chrome/browser/resources/chromeos/login/bubble.js:128: pos.right = origin.right
+ el.offsetWidth + PADDING;
pos.right = origin.left - PADDING?

http://codereview.chromium.org/9310050/diff/1/chrome/browser/resources/chrome...
File chrome/browser/resources/chromeos/login/display_manager.js (right):

http://codereview.chromium.org/9310050/diff/1/chrome/browser/resources/chrome...
chrome/browser/resources/chromeos/login/display_manager.js:320: bottom:
window.innerHeight - element.offsetHeight - offset.top,
What is the definition of the right and bottom here?
Shouldn't right = offset.left + element.offsetWidth and bottom = offset.top +
element.offsetHeight?

[Ivan Korotkov, 2012-02-03 10:33:04]

http://codereview.chromium.org/9310050/diff/1/chrome/browser/resources/chrome...
File chrome/browser/resources/chromeos/login/bubble.css (right):

http://codereview.chromium.org/9310050/diff/1/chrome/browser/resources/chrome...
chrome/browser/resources/chromeos/login/bubble.css:22: .bubble-right
On 2012/02/02 22:43:52, xiyuan wrote:
> nit: insert a blank line between css groups and all other similar places.

Done.

http://codereview.chromium.org/9310050/diff/1/chrome/browser/resources/chrome...
File chrome/browser/resources/chromeos/login/bubble.js (right):

http://codereview.chromium.org/9310050/diff/1/chrome/browser/resources/chrome...
chrome/browser/resources/chromeos/login/bubble.js:116: pos.bottom =
origin.bottom + el.offsetHeight + PADDING;
On 2012/02/02 22:43:52, xiyuan wrote:
> Should this be
> pos.bottom = origin.top - PADDING
> to make bubble sit on top of the anchor element?

This would not do the right thing. Imagine if origin.top is PADDING (anchor at
nearly top of the page), this would set pos.bottom = 0 (i.e., bubble at the
bottom of the page).

http://codereview.chromium.org/9310050/diff/1/chrome/browser/resources/chrome...
chrome/browser/resources/chromeos/login/bubble.js:128: pos.right = origin.right
+ el.offsetWidth + PADDING;
On 2012/02/02 22:43:52, xiyuan wrote:
> pos.right = origin.left - PADDING?

Same here.

http://codereview.chromium.org/9310050/diff/1/chrome/browser/resources/chrome...
File chrome/browser/resources/chromeos/login/display_manager.js (right):

http://codereview.chromium.org/9310050/diff/1/chrome/browser/resources/chrome...
chrome/browser/resources/chromeos/login/display_manager.js:320: bottom:
window.innerHeight - element.offsetHeight - offset.top,
On 2012/02/02 22:43:52, xiyuan wrote:
> What is the definition of the right and bottom here?
> Shouldn't right = offset.left + element.offsetWidth and bottom = offset.top +
> element.offsetHeight?

They are same as in CSS (http://www.w3schools.com/cssref/pr_pos_right.asp)

[altimofeev, 2012-02-03 11:49:30]

lgtm

http://codereview.chromium.org/9310050/diff/3003/chrome/browser/resources/chr...
File chrome/browser/resources/chromeos/login/bubble.css (right):

http://codereview.chromium.org/9310050/diff/3003/chrome/browser/resources/chr...
chrome/browser/resources/chromeos/login/bubble.css:23: .bubble-right
',' (comma) was missed

http://codereview.chromium.org/9310050/diff/3003/chrome/browser/resources/chr...
File chrome/browser/resources/chromeos/login/bubble.js (right):

http://codereview.chromium.org/9310050/diff/3003/chrome/browser/resources/chr...
chrome/browser/resources/chromeos/login/bubble.js:57: * Shows the bubble for
given anchor element.
nit: please update comment

http://codereview.chromium.org/9310050/diff/3003/chrome/browser/resources/chr...
chrome/browser/resources/chromeos/login/bubble.js:67: // Adjust position so that
arrow points exactly at |pos|.
nit: why not put ARROW_OFFSET to showContentForElement?

http://codereview.chromium.org/9310050/diff/3003/chrome/browser/resources/chr...
chrome/browser/resources/chromeos/login/bubble.js:70: if (pos.left)
please fix as we discussed (caller guarantees correctness)

http://codereview.chromium.org/9310050/diff/3003/chrome/browser/resources/chr...
File chrome/browser/resources/chromeos/login/display_manager.js (right):

http://codereview.chromium.org/9310050/diff/3003/chrome/browser/resources/chr...
chrome/browser/resources/chromeos/login/display_manager.js:298: * @return
{!Object} The offset (top, left, right, bottom).
Seems like it is not true.

http://codereview.chromium.org/9310050/diff/3003/chrome/browser/resources/chr...
File chrome/browser/resources/chromeos/login/oobe_screen_network.js (right):

http://codereview.chromium.org/9310050/diff/3003/chrome/browser/resources/chr...
chrome/browser/resources/chromeos/login/oobe_screen_network.js:95:
cr.ui.Bubble.Attachment.BOTTOM);
nit: maybe RIGHT ?

[xiyuan, 2012-02-03 18:06:23]

LGTM

[Ivan Korotkov, 2012-02-07 17:18:00]

Redesigned bubble for the new style (http://crosbug.com/21246) and fixed RTL
issues.
Care to take another look?

http://codereview.chromium.org/9310050/diff/3003/chrome/browser/resources/chr...
File chrome/browser/resources/chromeos/login/bubble.css (right):

http://codereview.chromium.org/9310050/diff/3003/chrome/browser/resources/chr...
chrome/browser/resources/chromeos/login/bubble.css:23: .bubble-right
On 2012/02/03 11:49:30, altimofeev wrote:
> ',' (comma) was missed

Done.

http://codereview.chromium.org/9310050/diff/3003/chrome/browser/resources/chr...
File chrome/browser/resources/chromeos/login/bubble.js (right):

http://codereview.chromium.org/9310050/diff/3003/chrome/browser/resources/chr...
chrome/browser/resources/chromeos/login/bubble.js:57: * Shows the bubble for
given anchor element.
On 2012/02/03 11:49:30, altimofeev wrote:
> nit: please update comment

Done.

http://codereview.chromium.org/9310050/diff/3003/chrome/browser/resources/chr...
chrome/browser/resources/chromeos/login/bubble.js:67: // Adjust position so that
arrow points exactly at |pos|.
On 2012/02/03 11:49:30, altimofeev wrote:
> nit: why not put ARROW_OFFSET to showContentForElement?

Makes sense.

http://codereview.chromium.org/9310050/diff/3003/chrome/browser/resources/chr...
chrome/browser/resources/chromeos/login/bubble.js:70: if (pos.left)
On 2012/02/03 11:49:30, altimofeev wrote:
> please fix as we discussed (caller guarantees correctness)

Done.

http://codereview.chromium.org/9310050/diff/3003/chrome/browser/resources/chr...
File chrome/browser/resources/chromeos/login/display_manager.js (right):

http://codereview.chromium.org/9310050/diff/3003/chrome/browser/resources/chr...
chrome/browser/resources/chromeos/login/display_manager.js:298: * @return
{!Object} The offset (top, left, right, bottom).
On 2012/02/03 11:49:30, altimofeev wrote:
> Seems like it is not true.

Fixed.

http://codereview.chromium.org/9310050/diff/3003/chrome/browser/resources/chr...
File chrome/browser/resources/chromeos/login/oobe_screen_network.js (right):

http://codereview.chromium.org/9310050/diff/3003/chrome/browser/resources/chr...
chrome/browser/resources/chromeos/login/oobe_screen_network.js:95:
cr.ui.Bubble.Attachment.BOTTOM);
On 2012/02/03 11:49:30, altimofeev wrote:
> nit: maybe RIGHT ?

It was bottom already, I think we don't want to change that...

[xiyuan, 2012-02-07 17:51:40]

LGTM as long as you did not change common.gypi in this CL. :)

http://codereview.chromium.org/9310050/diff/7001/build/common.gypi
File build/common.gypi (right):

http://codereview.chromium.org/9310050/diff/7001/build/common.gypi#newcode382
build/common.gypi:382: ['host_arch=="x64"', {
Is this change a result of rebase/merge?

http://codereview.chromium.org/9310050/diff/7001/chrome/browser/resources/chr...
File chrome/browser/resources/chromeos/login/bubble.css (right):

http://codereview.chromium.org/9310050/diff/7001/chrome/browser/resources/chr...
chrome/browser/resources/chromeos/login/bubble.css:11: /* border: 1px solid
rgba(0, 0, 0, 0.25); */
nit: Remove no-longer-needed code.

[Ivan Korotkov, 2012-02-07 17:57:27]

http://codereview.chromium.org/9310050/diff/7001/build/common.gypi
File build/common.gypi (right):

http://codereview.chromium.org/9310050/diff/7001/build/common.gypi#newcode382
build/common.gypi:382: ['host_arch=="x64"', {
On 2012/02/07 17:51:40, xiyuan wrote:
> Is this change a result of rebase/merge?

Yep, thanks.

http://codereview.chromium.org/9310050/diff/7001/chrome/browser/resources/chr...
File chrome/browser/resources/chromeos/login/bubble.css (right):

http://codereview.chromium.org/9310050/diff/7001/chrome/browser/resources/chr...
chrome/browser/resources/chromeos/login/bubble.css:11: /* border: 1px solid
rgba(0, 0, 0, 0.25); */
On 2012/02/07 17:51:40, xiyuan wrote:
> nit: Remove no-longer-needed code. 

Done.

[commit-bot: I haz the power, 2012-02-07 18:52:33]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/ivankr@chromium.org/9310050/7003

[commit-bot: I haz the power, 2012-02-07 23:36:01]

Change committed as 120854

[Dan Beam, 2012-12-05 07:23:47]

https://codereview.chromium.org/9310050/diff/7003/chrome/browser/resources/ch...
File chrome/browser/resources/chromeos/login/bubble.css (right):

https://codereview.chromium.org/9310050/diff/7003/chrome/browser/resources/ch...
chrome/browser/resources/chromeos/login/bubble.css:32: html[dir=ltf]
.bubble-top:before {
Hello Ivan, I noticed this says [dir=ltf] here and is still like this in the
code - what would you like to do about this rule?  I'd assume it hasn't been
working for these 10 months since this was committed.

Thanks,

Dan

[Ivan Korotkov, 2012-12-05 08:06:39]

https://codereview.chromium.org/9310050/diff/7003/chrome/browser/resources/ch...
File chrome/browser/resources/chromeos/login/bubble.css (right):

https://codereview.chromium.org/9310050/diff/7003/chrome/browser/resources/ch...
chrome/browser/resources/chromeos/login/bubble.css:32: html[dir=ltf]
.bubble-top:before {
On 2012/12/05 07:23:48, Dan Beam wrote:
> Hello Ivan, I noticed this says [dir=ltf] here and is still like this in the
> code - what would you like to do about this rule?  I'd assume it hasn't been
> working for these 10 months since this was committed.
> 
> Thanks,
> 
> Dan

Ouch. Thanks for noticing! I'll fix that.