Issue 2739473006: Update supervised user interstitial pages.

Bernhard Bauer

The CQ bit was checked by bauerb@chromium.org to run a CQ dry run

3 years, 9 months ago (2017-03-08 17:38:02 UTC) #1

commit-bot: I haz the power

Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2739473006/1

3 years, 9 months ago (2017-03-08 17:38:32 UTC) #2

commit-bot: I haz the power

The CQ bit was unchecked by commit-bot@chromium.org

3 years, 9 months ago (2017-03-08 18:26:24 UTC) #3

commit-bot: I haz the power

Dry run: This issue passed the CQ dry run.

3 years, 9 months ago (2017-03-08 18:26:25 UTC) #4

Bernhard Bauer

Description was changed from ========== Update supervised user interstitial pages. BUG=696536 ========== to ========== Update ...

3 years, 9 months ago (2017-03-08 18:38:33 UTC) #5

Bernhard Bauer

bauerb@chromium.org changed reviewers: + treib@chromium.org

3 years, 9 months ago (2017-03-08 18:38:57 UTC) #6

Marc Treib

Code LGTM with a few comments below. Some of the spacings look a bit off ...

3 years, 9 months ago (2017-03-09 09:55:58 UTC) #8

Bernhard Bauer

On 2017/03/09 09:55:58, Marc Treib wrote: > Code LGTM with a few comments below. > ...

3 years, 9 months ago (2017-03-10 10:12:10 UTC) #9

On 2017/03/09 09:55:58, Marc Treib wrote:
> Code LGTM with a few comments below.
> 
> Some of the spacings look a bit off to me (e.g. between custodian name and
> email), but I'll leave that to the UX folks.
> 
> Can we also get before/after screenshots for desktop? I can't predict what
that
> big blob of css changes will do there ;)

Good call! Comparing before and after turned up some regressions, which I've
fixed. Would you care to TAL? Updated screenshots are at
https://goo.gl/photos/Pz3quQ6XHdbiFRdc9.

https://codereview.chromium.org/2739473006/diff/1/components/supervised_user_...
File
components/supervised_user_error_page/resources/supervised_user_block_interstitial.css
(right):

https://codereview.chromium.org/2739473006/diff/1/components/supervised_user_...
components/supervised_user_error_page/resources/supervised_user_block_interstitial.css:92:
opacity: 0;
On 2017/03/09 09:55:57, Marc Treib wrote:
> This seems a bit odd - no better way to "hide" checkboxes?

As it turns out, there aren't actually any checkboxes in the interstitial, so I
removed it.

https://codereview.chromium.org/2739473006/diff/1/components/supervised_user_...
components/supervised_user_error_page/resources/supervised_user_block_interstitial.css:109:
display: inline;
On 2017/03/09 09:55:57, Marc Treib wrote:
> So what's the value otherwise? Why does it need to be changed based on the
> hidden/visible state?

By default it's displayed as a block element. The :not([hidden]) is just because
otherwise this selector would take precedence over the one from the built-in
stylesheet that hides anything with a [hidden] attribute.

https://codereview.chromium.org/2739473006/diff/1/components/supervised_user_...
components/supervised_user_error_page/resources/supervised_user_block_interstitial.css:196:
/*    display: block; */
On 2017/03/09 09:55:57, Marc Treib wrote:
> Remove comment

Done.

https://codereview.chromium.org/2739473006/diff/1/components/supervised_user_...
File
components/supervised_user_error_page/resources/supervised_user_block_interstitial.html
(right):

https://codereview.chromium.org/2739473006/diff/1/components/supervised_user_...
components/supervised_user_error_page/resources/supervised_user_block_interstitial.html:13:
<body id="body">
On 2017/03/09 09:55:57, Marc Treib wrote:
> nit: Is the id="body" actually needed for anything?

Not really; I had just copied that from the security interstitial.

https://codereview.chromium.org/2739473006/diff/1/components/supervised_user_...
components/supervised_user_error_page/resources/supervised_user_block_interstitial.html:14:
<div class="main-frame-blocked">
On 2017/03/09 09:55:57, Marc Treib wrote:
> Out of curiosity: Any particular reason for switching from id to class here
(and
> in a few other places)?

Mostly because that's what the security interstitial used. It's kind of a
tradeoff between making the stylesheet as similar as possible to the security
one (so we can merge them eventually) and making as few changes to this page as
possible, and in this instance I only had to change one thing here but would
have had to change the stylesheet in several places.

Bernhard Bauer

The CQ bit was checked by bauerb@chromium.org to run a CQ dry run

3 years, 9 months ago (2017-03-10 10:16:17 UTC) #10

commit-bot: I haz the power

Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2739473006/40001

3 years, 9 months ago (2017-03-10 10:16:31 UTC) #11

Marc Treib

Thanks! Still lgtm, modulo the CHROME_BUILD question below. [And I still think the spacing between ...

3 years, 9 months ago (2017-03-10 10:33:43 UTC) #12

commit-bot: I haz the power

The CQ bit was unchecked by commit-bot@chromium.org

3 years, 9 months ago (2017-03-10 10:34:54 UTC) #13

commit-bot: I haz the power

Dry run: Try jobs failed on following builders: ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/builds/170238)

3 years, 9 months ago (2017-03-10 10:34:55 UTC) #14

Bernhard Bauer

On 2017/03/10 10:33:43, Marc Treib wrote: > Thanks! Still lgtm, modulo the CHROME_BUILD question below. ...

3 years, 9 months ago (2017-03-10 11:56:42 UTC) #15

On 2017/03/10 10:33:43, Marc Treib wrote:
> Thanks! Still lgtm, modulo the CHROME_BUILD question below.
> 
> [And I still think the spacing between custodian name and email is weird. With
> two parents, would it look like this?
> 
> Parent 1
> 
> mailto:parent1@gmail.com
> Parent2
> 
> mailto:parent2@gmail.com
> ]

See https://goo.gl/photos/JsDVNobXA2FwXtjH7. I don't think it's too bad, but we
can definitely tweak it if necessary.

https://codereview.chromium.org/2739473006/diff/1/components/supervised_user_...
File
components/supervised_user_error_page/resources/supervised_user_block_interstitial.css
(right):

https://codereview.chromium.org/2739473006/diff/1/components/supervised_user_...
components/supervised_user_error_page/resources/supervised_user_block_interstitial.css:109:
display: inline;
On 2017/03/10 10:33:42, Marc Treib wrote:
> On 2017/03/10 10:12:10, Bernhard Bauer wrote:
> > On 2017/03/09 09:55:57, Marc Treib wrote:
> > > So what's the value otherwise? Why does it need to be changed based on the
> > > hidden/visible state?
> > 
> > By default it's displayed as a block element. The :not([hidden]) is just
> because
> > otherwise this selector would take precedence over the one from the built-in
> > stylesheet that hides anything with a [hidden] attribute.
> 
> So the "hidden" attribute effectively sets "display: none"?

Yes, exactly.

> So does that mean, anything that sets "display" here can not be hidden? That
> seems brittle...

Well, yes. Anything that changes the display attribute needs to make sure not to
override the [hidden] rule. I'm not sure there's a good alternative though: Even
if I used a custom class to set display: none, I would still need to make sure
that that rule doesn't get overridden. I have seen this done with !important,
but that seems like defeating the purpose of using cascading style sheets in the
first place.

https://codereview.chromium.org/2739473006/diff/40001/components/supervised_u...
File components/supervised_user_error_page/supervised_user_error_page.cc (left):

https://codereview.chromium.org/2739473006/diff/40001/components/supervised_u...
components/supervised_user_error_page/supervised_user_error_page.cc:128: #if
defined(GOOGLE_CHROME_BUILD)
On 2017/03/10 10:33:42, Marc Treib wrote:
> The reason for this was that feedback doesn't work in non-official builds. Has
> that changed?

I changed it so I can see the feedback link without having to build an
officially branded build. You are right that collecting feedback doesn't
actually work in a plain Chromium build (if nothing else because there is no
entity to collect feedback!), but AFAIK the actual requirements are slightly
different: On Android you need a Clankium build that is signed with a
first-party key (which I have), on desktop feedback is collected via an
extension... I don't know what the requirements for that are.

So I guess really we should ask the feedback mechanism whether it's functional
or not, and then show or hide the link based on that... followup? :)

Bernhard Bauer

The patchset sent to the CQ was uploaded after l-g-t-m from treib@chromium.org Link to the ...

3 years, 9 months ago (2017-03-10 11:59:00 UTC) #17

commit-bot: I haz the power

CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2739473006/60001

3 years, 9 months ago (2017-03-10 11:59:18 UTC) #18

Marc Treib

https://codereview.chromium.org/2739473006/diff/1/components/supervised_user_error_page/resources/supervised_user_block_interstitial.css File components/supervised_user_error_page/resources/supervised_user_block_interstitial.css (right): https://codereview.chromium.org/2739473006/diff/1/components/supervised_user_error_page/resources/supervised_user_block_interstitial.css#newcode109 components/supervised_user_error_page/resources/supervised_user_block_interstitial.css:109: display: inline; On 2017/03/10 11:56:42, Bernhard Bauer wrote: > ...

3 years, 9 months ago (2017-03-10 12:17:11 UTC) #19

https://codereview.chromium.org/2739473006/diff/1/components/supervised_user_...
File
components/supervised_user_error_page/resources/supervised_user_block_interstitial.css
(right):

https://codereview.chromium.org/2739473006/diff/1/components/supervised_user_...
components/supervised_user_error_page/resources/supervised_user_block_interstitial.css:109:
display: inline;
On 2017/03/10 11:56:42, Bernhard Bauer wrote:
> On 2017/03/10 10:33:42, Marc Treib wrote:
> > On 2017/03/10 10:12:10, Bernhard Bauer wrote:
> > > On 2017/03/09 09:55:57, Marc Treib wrote:
> > > > So what's the value otherwise? Why does it need to be changed based on
the
> > > > hidden/visible state?
> > > 
> > > By default it's displayed as a block element. The :not([hidden]) is just
> > because
> > > otherwise this selector would take precedence over the one from the
built-in
> > > stylesheet that hides anything with a [hidden] attribute.
> > 
> > So the "hidden" attribute effectively sets "display: none"?
> 
> Yes, exactly.
> 
> > So does that mean, anything that sets "display" here can not be hidden? That
> > seems brittle...
> 
> Well, yes. Anything that changes the display attribute needs to make sure not
to
> override the [hidden] rule. I'm not sure there's a good alternative though:
Even
> if I used a custom class to set display: none, I would still need to make sure
> that that rule doesn't get overridden. I have seen this done with !important,
> but that seems like defeating the purpose of using cascading style sheets in
the
> first place.

Yeah, I guess the hidden rule is effectively a custom class. Oh well.

https://codereview.chromium.org/2739473006/diff/40001/components/supervised_u...
File components/supervised_user_error_page/supervised_user_error_page.cc (left):

https://codereview.chromium.org/2739473006/diff/40001/components/supervised_u...
components/supervised_user_error_page/supervised_user_error_page.cc:128: #if
defined(GOOGLE_CHROME_BUILD)
On 2017/03/10 11:56:42, Bernhard Bauer wrote:
> On 2017/03/10 10:33:42, Marc Treib wrote:
> > The reason for this was that feedback doesn't work in non-official builds.
Has
> > that changed?
> 
> I changed it so I can see the feedback link without having to build an
> officially branded build. You are right that collecting feedback doesn't
> actually work in a plain Chromium build (if nothing else because there is no
> entity to collect feedback!), but AFAIK the actual requirements are slightly
> different: On Android you need a Clankium build that is signed with a
> first-party key (which I have), on desktop feedback is collected via an
> extension... I don't know what the requirements for that are.

Is it actually an extension? I thought at least some of the code was in Chromium
itself.
Anyway, GOOGLE_CHROME_BUILD should be a superset of the requirements, but I
guess it's not a big deal if the link shows up in other builds. (Those shouldn't
have signed-in child accounts anyway!)

> So I guess really we should ask the feedback mechanism whether it's functional
> or not, and then show or hide the link based on that... followup? :)

Sure, though I also don't mind too much if we just leave it as it is.

commit-bot: I haz the power

The CQ bit was unchecked by commit-bot@chromium.org

3 years, 9 months ago (2017-03-10 12:19:13 UTC) #20

commit-bot: I haz the power

Try jobs failed on following builders: ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/builds/170266)

3 years, 9 months ago (2017-03-10 12:19:14 UTC) #21

Bernhard Bauer

The patchset sent to the CQ was uploaded after l-g-t-m from treib@chromium.org Link to the ...

3 years, 9 months ago (2017-03-10 15:11:14 UTC) #23

commit-bot: I haz the power

CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2739473006/80001

3 years, 9 months ago (2017-03-10 15:11:27 UTC) #24

commit-bot: I haz the power

CQ is committing da patch. Bot data: {"patchset_id": 80001, "attempt_start_ts": 1489158673953470, "parent_rev": "ce0e63223261f496b2721a83c42aae8b163da960", "commit_rev": "679eb85f60078ada8e9c57cc56db76b6c4cee54f"}

3 years, 9 months ago (2017-03-10 16:18:57 UTC) #25

commit-bot: I haz the power

Description was changed from ========== Update supervised user interstitial pages. BUG=698235 ========== to ========== Update ...

3 years, 9 months ago (2017-03-10 16:19:35 UTC) #26

commit-bot: I haz the power

3 years, 9 months ago (2017-03-10 16:19:36 UTC) #27

Message was sent while issue was closed.

Committed patchset #5 (id:80001) as
https://chromium.googlesource.com/chromium/src/+/679eb85f60078ada8e9c57cc56db...

Issue 2739473006: Update supervised user interstitial pages. (Closed)

Description

Patch Set 1 #

Patch Set 2 : review #

Patch Set 3 : review #

Patch Set 4 : test #

Patch Set 5 : fix #

Messages