Embedded Extension Options: Improve the overlay popup UI

Embedded Extension Options: Improve the overlay popup UI

 - Add the extension's icon to the header
 - Add a drop shadow below the header to differentiate the extension
   content from the WebUI overlay

BUG=405796
Committed: https://crrev.com/0a553dff54bf9ca2d859e6595c2045c8d2e0785c
Cr-Commit-Position: refs/heads/master@{#291621}

[not at google - send to devlin, 2014-08-22 22:31:01]

https://codereview.chromium.org/488293003/diff/1/chrome/browser/resources/ext...
File chrome/browser/resources/extensions/extension_options_overlay.css (right):

https://codereview.chromium.org/488293003/diff/1/chrome/browser/resources/ext...
chrome/browser/resources/extensions/extension_options_overlay.css:18: z-index:
1;
Why the z-indices?

https://codereview.chromium.org/488293003/diff/1/chrome/browser/resources/ext...
chrome/browser/resources/extensions/extension_options_overlay.css:27: float:
left;
Why float?

https://codereview.chromium.org/488293003/diff/1/chrome/browser/resources/ext...
chrome/browser/resources/extensions/extension_options_overlay.css:33:
#extension-options-overlay-icon img {
Why meddling with the display everywhere?

https://codereview.chromium.org/488293003/diff/1/chrome/browser/resources/ext...
chrome/browser/resources/extensions/extension_options_overlay.css:42: }
I should come around to your desk and we can verify all this CSS is necessary.

https://codereview.chromium.org/488293003/diff/1/chrome/browser/resources/ext...
File chrome/browser/resources/extensions/extension_options_overlay.html (right):

https://codereview.chromium.org/488293003/diff/1/chrome/browser/resources/ext...
chrome/browser/resources/extensions/extension_options_overlay.html:7: <div
class="close-button"></div>
The close button is on the right side of the box, correct? It should be last in
the DOM then. Perhaps that would solve some of the float/z-index issues.

https://codereview.chromium.org/488293003/diff/1/chrome/browser/resources/ext...
File chrome/browser/resources/extensions/extension_options_overlay.js (right):

https://codereview.chromium.org/488293003/diff/1/chrome/browser/resources/ext...
chrome/browser/resources/extensions/extension_options_overlay.js:56:
$('extension-options-overlay-icon').removeChild(icon);
Should we basically be removing #extension-options-overlay from the DOM? Or at
least all its children?

https://codereview.chromium.org/488293003/diff/1/chrome/browser/resources/ext...
chrome/browser/resources/extensions/extension_options_overlay.js:74:
$('extension-options-overlay-icon').appendChild(icon);
Why not always have the img in the DOM, then set the src when it's shown? Also,
make sure it has empty alt text.

[ericzeng, 2014-08-23 00:14:50]

https://codereview.chromium.org/488293003/diff/1/chrome/browser/resources/ext...
File chrome/browser/resources/extensions/extension_options_overlay.css (right):

https://codereview.chromium.org/488293003/diff/1/chrome/browser/resources/ext...
chrome/browser/resources/extensions/extension_options_overlay.css:18: z-index:
1;
On 2014/08/22 22:31:00, kalman wrote:
> Why the z-indices?

I needed a z-index on this div so that I could line up the heading text and the
icon. Since close-button also has a z-index of 1 I had to make my close-button
have a z-index of 2.

https://codereview.chromium.org/488293003/diff/1/chrome/browser/resources/ext...
chrome/browser/resources/extensions/extension_options_overlay.css:27: float:
left;
On 2014/08/22 22:31:00, kalman wrote:
> Why float?

Also needed it to align the heading and icon

https://codereview.chromium.org/488293003/diff/1/chrome/browser/resources/ext...
chrome/browser/resources/extensions/extension_options_overlay.css:33:
#extension-options-overlay-icon img {
On 2014/08/22 22:31:00, kalman wrote:
> Why meddling with the display everywhere?

Alignment aarrrrgghhh

https://codereview.chromium.org/488293003/diff/1/chrome/browser/resources/ext...
File chrome/browser/resources/extensions/extension_options_overlay.html (right):

https://codereview.chromium.org/488293003/diff/1/chrome/browser/resources/ext...
chrome/browser/resources/extensions/extension_options_overlay.html:7: <div
class="close-button"></div>
On 2014/08/22 22:31:00, kalman wrote:
> The close button is on the right side of the box, correct? It should be last
in
> the DOM then. Perhaps that would solve some of the float/z-index issues.

The close button seems to find the right spot in the overlay no matter where in
the DOM I put it.

https://codereview.chromium.org/488293003/diff/1/chrome/browser/resources/ext...
File chrome/browser/resources/extensions/extension_options_overlay.js (right):

https://codereview.chromium.org/488293003/diff/1/chrome/browser/resources/ext...
chrome/browser/resources/extensions/extension_options_overlay.js:56:
$('extension-options-overlay-icon').removeChild(icon);
On 2014/08/22 22:31:00, kalman wrote:
> Should we basically be removing #extension-options-overlay from the DOM? Or at
> least all its children?

No, every overlay's nodes remain in the DOM (not just this one), but the parent
overlay div's visibility is toggled.

https://codereview.chromium.org/488293003/diff/1/chrome/browser/resources/ext...
chrome/browser/resources/extensions/extension_options_overlay.js:74:
$('extension-options-overlay-icon').appendChild(icon);
On 2014/08/22 22:31:00, kalman wrote:
> Why not always have the img in the DOM, then set the src when it's shown?
Also,
> make sure it has empty alt text.

Done

[not at google - send to devlin, 2014-08-23 00:24:16]

lgtm

https://codereview.chromium.org/488293003/diff/20001/chrome/browser/resources...
File chrome/browser/resources/extensions/extension_options_overlay.html (right):

https://codereview.chromium.org/488293003/diff/20001/chrome/browser/resources...
chrome/browser/resources/extensions/extension_options_overlay.html:9: <img
id="extension-options-overlay-icon"></img>
alt=""

https://codereview.chromium.org/488293003/diff/20001/chrome/browser/resources...
File chrome/browser/resources/extensions/extension_options_overlay.js (right):

https://codereview.chromium.org/488293003/diff/20001/chrome/browser/resources...
chrome/browser/resources/extensions/extension_options_overlay.js:69:
$('extension-options-overlay-icon').src = extensionIcon;
Should this be .setAttribute('src')? Even if .src works, setAttribute would be
consistent with removeAttribute.

[ericzeng, 2014-08-24 04:59:52]

The CQ bit was checked by ericzeng@chromium.org

[commit-bot: I haz the power, 2014-08-24 05:00:56]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/ericzeng@chromium.org/488293003/80001

[ericzeng, 2014-08-24 05:51:36]

The CQ bit was unchecked by ericzeng@chromium.org

[ericzeng, 2014-08-24 05:52:13]

The CQ bit was checked by ericzeng@chromium.org

[commit-bot: I haz the power, 2014-08-24 05:53:18]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/ericzeng@chromium.org/488293003/80001

[commit-bot: I haz the power, 2014-08-24 05:53:37]

Committed patchset #5 (80001) as a428f7f6c077e0f27f35f3e20fe709eb328fe9d6

[commit-bot: I haz the power, 2014-09-10 02:32:25]

Patchset 5 (id:??) landed as
https://crrev.com/0a553dff54bf9ca2d859e6595c2045c8d2e0785c
Cr-Commit-Position: refs/heads/master@{#291621}