Controlled settings indicator and bubble for chrome://preferences.

Controlled settings indicator and bubble for chrome://preferences.

The idea is that every setting that is under control by an external entity (e.g. policy, extension) will be annotated with an icon that, when click on, will show a bubble with more information. This implements the indicator icon and bubble. Going through the prefs UI and inserting the thing at the appropriate places is left for subsequent CLs :)

BUG=chromium:104955
TEST=No observable changes.

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

[Patrick Dubroy, 2011-11-21 12:41:16]

http://codereview.chromium.org/8480037/diff/1/chrome/browser/resources/option...
File chrome/browser/resources/options/controlled_setting.js (right):

http://codereview.chromium.org/8480037/diff/1/chrome/browser/resources/option...
chrome/browser/resources/options/controlled_setting.js:27: // If there is a
pref, track it's controlledBy property in order to be
its, not it's.

http://codereview.chromium.org/8480037/diff/1/chrome/browser/resources/option...
chrome/browser/resources/options/controlled_setting.js:71: var bubbleSpan =
doc.createElement('div');
Is bubbleSpan the right name for this? :-)

http://codereview.chromium.org/8480037/diff/1/chrome/browser/resources/option...
chrome/browser/resources/options/controlled_setting.js:76:
bubbleText.appendChild(doc.createTextNode(text));
Wouldn't it be simpler to use:

    bubbleText.textContent = text;

http://codereview.chromium.org/8480037/diff/1/chrome/browser/resources/option...
chrome/browser/resources/options/controlled_setting.js:91: e.stopPropagation();
Why do you need to stop propagation here?

http://codereview.chromium.org/8480037/diff/1/chrome/browser/resources/option...
chrome/browser/resources/options/controlled_setting.js:114: hide_: function() {
I think it would be better to just remove the elements that you created.
Otherwise, it might be confusing someone tries to add other children to the
span, and they are mysteriously deleted.

http://codereview.chromium.org/8480037/diff/1/chrome/browser/resources/option...
File chrome/browser/resources/options/options_page.css (right):

http://codereview.chromium.org/8480037/diff/1/chrome/browser/resources/option...
chrome/browser/resources/options/options_page.css:617: background-size: contain;
Do you need this? It will cause the image to scale if it's smaller than this
element, which we probably don't want.

http://codereview.chromium.org/8480037/diff/1/chrome/browser/resources/option...
chrome/browser/resources/options/options_page.css:620: position: relative;
Maybe add a comment to explain why position:relative is needed here.

[Mattias Nissler (ping if slow), 2011-11-21 13:45:01]

James, can you review? Thanks.

Note that there are some image files pending that I need to get from UI first,
but except for that, this is ready.

http://codereview.chromium.org/8480037/diff/1/chrome/browser/resources/option...
File chrome/browser/resources/options/controlled_setting.js (right):

http://codereview.chromium.org/8480037/diff/1/chrome/browser/resources/option...
chrome/browser/resources/options/controlled_setting.js:27: // If there is a
pref, track it's controlledBy property in order to be
On 2011/11/21 12:41:16, dubroy wrote:
> its, not it's.

Done.

http://codereview.chromium.org/8480037/diff/1/chrome/browser/resources/option...
chrome/browser/resources/options/controlled_setting.js:71: var bubbleSpan =
doc.createElement('div');
On 2011/11/21 12:41:16, dubroy wrote:
> Is bubbleSpan the right name for this? :-)

Changed to bubbleContainer.

http://codereview.chromium.org/8480037/diff/1/chrome/browser/resources/option...
chrome/browser/resources/options/controlled_setting.js:76:
bubbleText.appendChild(doc.createTextNode(text));
On 2011/11/21 12:41:16, dubroy wrote:
> Wouldn't it be simpler to use:
> 
>     bubbleText.textContent = text;

I was writing this when I had inserted more children before ;)
Done.

http://codereview.chromium.org/8480037/diff/1/chrome/browser/resources/option...
chrome/browser/resources/options/controlled_setting.js:91: e.stopPropagation();
On 2011/11/21 12:41:16, dubroy wrote:
> Why do you need to stop propagation here?

Because otherwise the click handler on the icon re-builds the bubble (which I
figured is a good thing if people click on the icon again in case the pref
changed to be controlled by something else).

http://codereview.chromium.org/8480037/diff/1/chrome/browser/resources/option...
chrome/browser/resources/options/controlled_setting.js:114: hide_: function() {
On 2011/11/21 12:41:16, dubroy wrote:
> I think it would be better to just remove the elements that you created.
> Otherwise, it might be confusing someone tries to add other children to the
> span, and they are mysteriously deleted.

Done.

http://codereview.chromium.org/8480037/diff/1/chrome/browser/resources/option...
File chrome/browser/resources/options/options_page.css (right):

http://codereview.chromium.org/8480037/diff/1/chrome/browser/resources/option...
chrome/browser/resources/options/options_page.css:617: background-size: contain;
On 2011/11/21 12:41:16, dubroy wrote:
> Do you need this? It will cause the image to scale if it's smaller than this
> element, which we probably don't want.

The scaling is exactly why I put it. The reason is that we'll need the indicator
in different places at different sizes. We could include properly-sized images
for all that, but we'll still have the controlledBy x size matrix and have to
write CSS rules for each combination. So I figured having to write rules only
for controlledBy and having the browser scale the image to the right size is a
reasonable trade-off in this case. Let me know if you disagree.

http://codereview.chromium.org/8480037/diff/1/chrome/browser/resources/option...
chrome/browser/resources/options/options_page.css:620: position: relative;
On 2011/11/21 12:41:16, dubroy wrote:
> Maybe add a comment to explain why position:relative is needed here.

Done.

[Patrick Dubroy, 2011-11-21 13:53:19]

http://codereview.chromium.org/8480037/diff/1/chrome/browser/resources/option...
File chrome/browser/resources/options/options_page.css (right):

http://codereview.chromium.org/8480037/diff/1/chrome/browser/resources/option...
chrome/browser/resources/options/options_page.css:617: background-size: contain;
On 2011/11/21 13:45:01, Mattias Nissler wrote:
> On 2011/11/21 12:41:16, dubroy wrote:
> > Do you need this? It will cause the image to scale if it's smaller than this
> > element, which we probably don't want.
> 
> The scaling is exactly why I put it. The reason is that we'll need the
indicator
> in different places at different sizes. We could include properly-sized images
> for all that, but we'll still have the controlledBy x size matrix and have to
> write CSS rules for each combination. So I figured having to write rules only
> for controlledBy and having the browser scale the image to the right size is a
> reasonable trade-off in this case. Let me know if you disagree.

No, that makes sense. I realized why you did it after I saw the screenshots.

http://codereview.chromium.org/8480037/diff/4011/chrome/browser/resources/opt...
File chrome/browser/resources/options/controlled_setting.js (right):

http://codereview.chromium.org/8480037/diff/4011/chrome/browser/resources/opt...
chrome/browser/resources/options/controlled_setting.js:91: // Stop propagation
s.t. the click handler on the indicator icon
Nit: I don't think this is worth abbreviating. It took me a sec to realize that
"s.t." was short for "so that". AFAIK that's not a common abbreviation.

[Mattias Nissler (ping if slow), 2011-11-28 17:48:28]

James: friendly ping.

[Mattias Nissler (ping if slow), 2011-11-28 21:44:28]

Ah, I had the wrong James on the reviewers list. No surprise he doesn't answer
;)

James, can you take a look at this? Thanks!

[James Hawkins, 2011-11-28 21:52:19]

http://codereview.chromium.org/8480037/diff/22001/chrome/browser/resources/op...
File chrome/browser/resources/options/controlled_setting.js (right):

http://codereview.chromium.org/8480037/diff/22001/chrome/browser/resources/op...
chrome/browser/resources/options/controlled_setting.js:6: 
Remove blank line.

http://codereview.chromium.org/8480037/diff/22001/chrome/browser/resources/op...
File chrome/browser/resources/options/options_page.css (right):

http://codereview.chromium.org/8480037/diff/22001/chrome/browser/resources/op...
chrome/browser/resources/options/options_page.css:629:
.controlled-setting-indicator summary::-webkit-details-marker {
The double colon is not necessary, right?

http://codereview.chromium.org/8480037/diff/22001/chrome/browser/resources/op...
chrome/browser/resources/options/options_page.css:688: border-width: 0px 5px
5px;
s/0px/0/ here and elsewhere.

http://codereview.chromium.org/8480037/diff/22001/chrome/browser/resources/op...
chrome/browser/resources/options/options_page.css:689: content: '';
Do you need to specify the empty content?

[Mattias Nissler (ping if slow), 2011-11-28 22:26:19]

PTAL. Thanks!

http://codereview.chromium.org/8480037/diff/22001/chrome/browser/resources/op...
File chrome/browser/resources/options/controlled_setting.js (right):

http://codereview.chromium.org/8480037/diff/22001/chrome/browser/resources/op...
chrome/browser/resources/options/controlled_setting.js:6: 
On 2011/11/28 21:52:19, James Hawkins wrote:
> Remove blank line.

Done.

http://codereview.chromium.org/8480037/diff/22001/chrome/browser/resources/op...
File chrome/browser/resources/options/options_page.css (right):

http://codereview.chromium.org/8480037/diff/22001/chrome/browser/resources/op...
chrome/browser/resources/options/options_page.css:629:
.controlled-setting-indicator summary::-webkit-details-marker {
On 2011/11/28 21:52:19, James Hawkins wrote:
> The double colon is not necessary, right?

Actually, it is. At least the rule doesn't match if I remove one colon... 

Looking at the spec, this is actually a pseudo-element
(http://www.w3.org/TR/selectors/#pseudo-elements). In contrast to
pseudo-classes, pseudo-elements are declared with two colons. Turns out :before
and :after are as well, so I've changed these to two-colon versions below :)

http://codereview.chromium.org/8480037/diff/22001/chrome/browser/resources/op...
chrome/browser/resources/options/options_page.css:688: border-width: 0px 5px
5px;
On 2011/11/28 21:52:19, James Hawkins wrote:
> s/0px/0/ here and elsewhere.

Done.

http://codereview.chromium.org/8480037/diff/22001/chrome/browser/resources/op...
chrome/browser/resources/options/options_page.css:689: content: '';
On 2011/11/28 21:52:19, James Hawkins wrote:
> Do you need to specify the empty content?

Yes, the element doesn't get rendered otherwise.

[James Hawkins, 2011-11-28 22:41:22]

LGTM, thanks.