Issue 265393005: Implement apps v2 custom window frame.

Issue 265393005: Implement apps v2 custom window frame. (Closed)

Created:
6 years, 7 months ago by Jamie

Modified:
6 years, 7 months ago

Reviewers:
Sergey Ulanov, garykac, kelvinp

CC:
chromium-reviews, dcheng, chromoting-reviews_chromium.org

Base URL:
svn://svn.chromium.org/chrome/trunk/src

Visibility:
Public.

More Reviews

Description

Implement apps v2 custom window frame. The default apps v2 container is pretty basic. We want to provide something that looks prettier and also implements some functionality specific to our use-case: * When connected to a host, a disconnect icon is added to the window controls (it's therefore no longer needed in the tool-bar). * When connected to a host, maximize == full-screen. * In full-screen mode, the window controls are still accessible, but are auto-hidden near the top-left corner (but not obscuring it, since it's often a hot-spot on the server). * For touch-screen devices with no concept of hover, clicking the "stub" will also reveal the controls. There should be no change to the v1 UX, but I don't plan on landing this CL before the M36 branch point, just in case. BUG=134213 Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=270342

Patch Set 1 #

Patch Set 2 : Removed file added accidentally. #

Total comments: 38

Patch Set 3 : Reviewer feedback. #

Patch Set 4 : Remove unnecessary getElementById and work-around AppWindow.restore bug. #

Total comments: 2

Patch Set 5 : Added RTL support and fixed border and drop-shadow CSS. #

Patch Set 6 : Improve disconnect icon. #

Patch Set 7 : Rebase #

Created: 6 years, 7 months ago

Download [raw] [tar.bz2]

	Unified diffs	Side-by-side diffs	Delta from patch set	Stats (+533 lines, -81 lines)			Patch
M	remoting/remoting.gyp	View	1 2 3 4 5 6	1 chunk	+1 line, -0 lines	0 comments	Download
M	remoting/remoting_webapp_files.gypi	View	1 2 3 4 5 6	4 chunks	+8 lines, -0 lines	0 comments	Download
A	remoting/resources/drag.webp	View		Binary file		0 comments	Download
A	remoting/resources/icon_close.webp	View		Binary file		0 comments	Download
A	remoting/resources/icon_disconnect.webp	View	1 2 3 4 5	Binary file		0 comments	Download
A	remoting/resources/icon_maximize_restore.webp	View		Binary file		0 comments	Download
A	remoting/resources/icon_minimize.webp	View		Binary file		0 comments	Download
M	remoting/resources/remoting_strings.grd	View	1 2 3 4 5 6	1 chunk	+16 lines, -1 line	0 comments	Download
M	remoting/webapp/background.js	View		1 chunk	+2 lines, -1 line	0 comments	Download
M	remoting/webapp/client_session.js	View	1 2 3 4 5 6	11 chunks	+56 lines, -30 lines	0 comments	Download
M	remoting/webapp/event_handlers.js	View		1 chunk	+6 lines, -0 lines	0 comments	Download
M	remoting/webapp/fullscreen_v2.js	View		2 chunks	+2 lines, -0 lines	0 comments	Download
M	remoting/webapp/html/template_main.html	View	1 2	3 chunks	+49 lines, -43 lines	0 comments	Download
M	remoting/webapp/html/toolbar.html	View		2 chunks	+8 lines, -4 lines	0 comments	Download
A	remoting/webapp/html/window_frame.html	View	1 2	1 chunk	+29 lines, -0 lines	0 comments	Download
M	remoting/webapp/js_proto/chrome_proto.js	View		1 chunk	+1 line, -0 lines	0 comments	Download
M	remoting/webapp/main.css	View	1 2 3 4 5 6	3 chunks	+3 lines, -1 line	0 comments	Download
M	remoting/webapp/remoting.js	View	1 2 3 4 5 6	1 chunk	+2 lines, -0 lines	0 comments	Download
M	remoting/webapp/toolbar.css	View		1 chunk	+1 line, -1 line	0 comments	Download
A	remoting/webapp/window_frame.css	View	1 2 3 4	1 chunk	+174 lines, -0 lines	0 comments	Download
A	remoting/webapp/window_frame.js	View	1 2 3	1 chunk	+175 lines, -0 lines	0 comments	Download

Messages

Total messages: 20 (0 generated)

Expand Messages | Collapse Messages

Jamie

https://codereview.chromium.org/265393005/diff/20001/remoting/webapp/html/title_bar.html File remoting/webapp/html/title_bar.html (right): https://codereview.chromium.org/265393005/diff/20001/remoting/webapp/html/title_bar.html#newcode7 remoting/webapp/html/title_bar.html:7: <span id="window-title" class="window-title"> </span> I don't like the use of ...

6 years, 7 months ago (2014-05-05 22:37:15 UTC) #1

kelvinp

https://codereview.chromium.org/265393005/diff/20001/remoting/resources/remoting_strings.grd File remoting/resources/remoting_strings.grd (right): https://codereview.chromium.org/265393005/diff/20001/remoting/resources/remoting_strings.grd#newcode491 remoting/resources/remoting_strings.grd:491: <message desc="Tool-top for the window's close icon." name="IDS_CLOSE_WINDOW"> s/top/tip ...

6 years, 7 months ago (2014-05-06 04:42:15 UTC) #2

Sergey Ulanov

https://codereview.chromium.org/265393005/diff/20001/remoting/resources/remoting_strings.grd File remoting/resources/remoting_strings.grd (right): https://codereview.chromium.org/265393005/diff/20001/remoting/resources/remoting_strings.grd#newcode713 remoting/resources/remoting_strings.grd:713: <message desc="Tool-tip for the window's restore icon." name="IDS_RESTORE_WINDOW"> Some ...

6 years, 7 months ago (2014-05-06 07:58:14 UTC) #3

Jamie

6 years, 7 months ago (2014-05-06 21:11:07 UTC) #4

https://codereview.chromium.org/265393005/diff/20001/remoting/resources/remot...
File remoting/resources/remoting_strings.grd (right):

https://codereview.chromium.org/265393005/diff/20001/remoting/resources/remot...
remoting/resources/remoting_strings.grd:491: <message desc="Tool-top for the
window's close icon." name="IDS_CLOSE_WINDOW">
On 2014/05/06 04:42:15, kelvinp wrote:
> s/top/tip

Done.

https://codereview.chromium.org/265393005/diff/20001/remoting/resources/remot...
remoting/resources/remoting_strings.grd:713: <message desc="Tool-tip for the
window's restore icon." name="IDS_RESTORE_WINDOW">
On 2014/05/06 07:58:14, Sergey Ulanov wrote:
> Some of the strings in this file are sorted by alphabet, but it's just
> side-effect from being generated from .json files in the past. IMO it's not
> really useful to sort strings by alphabet and it's better to group them by the
> UI partition they belong to. 
> Or if you prefer alphabet ordering, then it might be useful to add a common
> suffix, e.g. IDS_TITLE_BAR_RESTORE_WINDOW, then all strings will be together.

Done.

https://codereview.chromium.org/265393005/diff/20001/remoting/webapp/html/tit...
File remoting/webapp/html/title_bar.html (right):

https://codereview.chromium.org/265393005/diff/20001/remoting/webapp/html/tit...
remoting/webapp/html/title_bar.html:7: <span id="window-title"
class="window-title">&nbsp;</span>
On 2014/05/06 07:58:14, Sergey Ulanov wrote:
> On 2014/05/05 22:37:15, Jamie wrote:
> > I don't like the use of &nbsp; here, but the size specifications for the
span
> > are not respected without it. If you can think of a better solution, please
> let
> > me know.
> 
> Does it not work if you put a space instead of &nbsp; here?

I don't think so. Even if it did, I would be nervous about it being removed by a
future edit that thought it was unnecessary white-space.

https://codereview.chromium.org/265393005/diff/20001/remoting/webapp/html/tit...
remoting/webapp/html/title_bar.html:7: <span id="window-title"
class="window-title">&nbsp;</span>
On 2014/05/06 07:58:14, Sergey Ulanov wrote:
> nit: do you need both class and id here?

I've eliminated all ids except the top-level one in favour of a pattern whereby
the root node is passed to the WindowFrame class, which uses querySelector to
find the child nodes.

https://codereview.chromium.org/265393005/diff/20001/remoting/webapp/html/tit...
remoting/webapp/html/title_bar.html:8: <span
class="window-controls-hover-target" id="window-controls">
On 2014/05/06 07:58:14, Sergey Ulanov wrote:
> Maybe use divs instead of spans and explicitly set display to inline-block?
> Otherwise presentation is not separated from the content. Also it's confusing
> when divs are mixed with spans. 

I've used spans for content that is laid out horizontally and divs for content
that is laid out vertically. I think that more clearly describes the layout than
using divs everywhere. LMK if you disagree; the CSS overrides the defaults
sufficiently that I don't think it makes any difference and I don't have a
strong preference either way.

It turns out there was an extra unnecessary div, which I've removed, but I've
left everything else unchanged.

https://codereview.chromium.org/265393005/diff/20001/remoting/webapp/html/tit...
remoting/webapp/html/title_bar.html:9: <span class="window-controls">
On 2014/05/06 07:58:14, Sergey Ulanov wrote:
> you have both id=window-controls and class=window-controls. This might be
> confusing and I'm not sure if id=window-controls is useful at all.

See above.

https://codereview.chromium.org/265393005/diff/20001/remoting/webapp/html/tit...
remoting/webapp/html/title_bar.html:10: <div>
On 2014/05/06 04:42:15, kelvinp wrote:
> <span>'s are inline elements and <div>'s are block elements.  Is it a
particular
> reason why the div is sandwiched between the spans here?  Can we replace the
div
> with a span?

Done.

https://codereview.chromium.org/265393005/diff/20001/remoting/webapp/html/tit...
remoting/webapp/html/title_bar.html:16: <span id="window-minimize"
On 2014/05/06 04:42:15, kelvinp wrote:
> Do we care about RTL languages?  Do we need to show the buttons on the
top-right
> corner on those locales?

Good catch! We do care, and I think the correct behaviour is to lay out the
controls left-aligned. I'm not certain though, and it might be a bit fiddly to
get it working, so I would prefer to follow up with a CL to address just that
after confirming that it's the right behaviour.

https://codereview.chromium.org/265393005/diff/20001/remoting/webapp/title_ba...
File remoting/webapp/title_bar.css (right):

https://codereview.chromium.org/265393005/diff/20001/remoting/webapp/title_ba...
remoting/webapp/title_bar.css:1: html.apps-v2,
On 2014/05/06 07:58:14, Sergey Ulanov wrote:
> add copyright header

Done.

https://codereview.chromium.org/265393005/diff/20001/remoting/webapp/title_ba...
remoting/webapp/title_bar.css:8: border: 1px solid gray;  /** This is the window
border. */
On 2014/05/06 07:58:14, Sergey Ulanov wrote:
> why "/**" instead of "/*?

Force of habit. Done.

https://codereview.chromium.org/265393005/diff/20001/remoting/webapp/title_ba...
remoting/webapp/title_bar.css:39: display: table;
On 2014/05/06 04:42:15, kelvinp wrote:
> I think we can probably achieve the same layout without using display:table. 
Is
> that a particular reason why we need to use a table here?

See my response to Sergey's similar comment.

https://codereview.chromium.org/265393005/diff/20001/remoting/webapp/title_ba...
remoting/webapp/title_bar.css:68: -webkit-column-span: all;
On 2014/05/06 07:58:14, Sergey Ulanov wrote:
> is it possible to use flex instead of tables to layout controls?

I tried lots of different methods for laying out the controls, and all had
subtle problems I couldn't work-around except for tables. The most common was a
gap between the buttons, but tbh I don't remember all the failure modes, or if I
even tried flexbox.

I think it's important to distinguish between using <table> as a cheat to get
the layout you want vs. using "display: table" in the CSS. Using <table> for
something that is not tabular content pollutes the mark-up and should be
avoided; OTOH, "display: table", IMO, is fine if it's the clearest way of
expressing how things should look.

I would prefer not to spend a lot of time trying to get this working with
flexbox, but LMK if you feel strongly that we should not be using "display:
table".

https://codereview.chromium.org/265393005/diff/20001/remoting/webapp/title_ba...
remoting/webapp/title_bar.css:143: border-bottom: none;
On 2014/05/06 07:58:14, Sergey Ulanov wrote:
> nit: indentation

Done.

https://codereview.chromium.org/265393005/diff/20001/remoting/webapp/title_ba...
remoting/webapp/title_bar.css:151: -webkit-transition-property: opacity,
On 2014/05/06 07:58:14, Sergey Ulanov wrote:
> CSS transitions no longer need the prefix.

Done.

https://codereview.chromium.org/265393005/diff/20001/remoting/webapp/title_ba...
remoting/webapp/title_bar.css:172: -webkit-box-shadow: 1px 1px 10px rgba(0, 0,
0, 0.5);
On 2014/05/06 07:58:14, Sergey Ulanov wrote:
> don't need -webkit-

Done.

https://codereview.chromium.org/265393005/diff/20001/remoting/webapp/title_ba...
File remoting/webapp/title_bar.js (right):

https://codereview.chromium.org/265393005/diff/20001/remoting/webapp/title_ba...
remoting/webapp/title_bar.js:23: this.clientSession_ = null;
On 2014/05/06 04:42:15, kelvinp wrote:
> It seems like this is only used as a boolean to indicate whether we are
> connected or not.  IMO, I think the TitleBar should as at little knowledge
about
> the app as possible.
> 
> What about exposing method called showDisconnectButton on the title bar
instead,
> so that the ClientSession can call it when the connection state changes?
> 

I've renamed it to setConnected. I had originally wanted a reference to the
client session because it has a disconnect method. Unfortunately, that method
doesn't do the necessary DOM clean-up, so we have to call the global
remoting.disconnect instead, which is ugly but it does mean that the client
session reference is no longer needed, as you point out.

https://codereview.chromium.org/265393005/diff/20001/remoting/webapp/title_ba...
remoting/webapp/title_bar.js:69: remoting.TitleBar.prototype.getClientArea =
function() {
On 2014/05/06 04:42:15, kelvinp wrote:
> It seems a bit odd to have a getClientArea method on a class called TitleBar. 
I
> think remoting.AppWindow seems like a better name for this class. 

I've gone with WindowFrame instead, since AppWindow is already a class in the
chrome namespace with very different semantics.

https://codereview.chromium.org/265393005/diff/20001/remoting/webapp/title_ba...
remoting/webapp/title_bar.js:96: chrome.app.window.current().isFullscreen() ||
On 2014/05/06 04:42:15, kelvinp wrote:
> It seems a bit odd that some of the fullscreen management is done via
> remoting.fullscreen but some is done through chrome.app.window.current().  Can
> we unify the two?  May be remoting.TitleBar can call into remoting.fullScreen?

Since we don't encapsulate the maximized state, I think it would be more strange
to use remoting.FullScreen for the full-screen state, but chrome.AppWindow for
the maximized state. I prefer to leave this as-it; LMK if you feel strongly
about it.

Jamie

A couple more changes: Patchset 4 is an ugly hack to work around what appears ...

6 years, 7 months ago (2014-05-07 01:23:13 UTC) #8

kelvinp

lgtm Thx for doing the RTL support. Looks good. Just one minor comment. https://codereview.chromium.org/265393005/diff/60001/remoting/webapp/window_frame.js File ...

6 years, 7 months ago (2014-05-07 18:44:11 UTC) #9

kelvinp

lgtm Thx for doing the RTL support. Looks good. Just one minor comment.

6 years, 7 months ago (2014-05-07 18:44:14 UTC) #10

Jamie

fyi https://codereview.chromium.org/265393005/diff/60001/remoting/webapp/window_frame.js File remoting/webapp/window_frame.js (right): https://codereview.chromium.org/265393005/diff/60001/remoting/webapp/window_frame.js#newcode109 remoting/webapp/window_frame.js:109: // When the user disconnects, exit full-screen mode. ...

6 years, 7 months ago (2014-05-07 19:31:15 UTC) #11