Navigation transitions: Hide transition elements by setting CSS opacity rather than display.

Navigation transitions: Hide transition elements by setting opacity: 0 rather than display: none, to not shift other elements around.

Also removed a redundant parameter from Internals::serializeNavigationMarkup.

R=abarth
BUG=370696
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=180318

[oystein (OOO til 10th of July), 2014-08-14 21:04:47]

ptal :)

[abarth-chromium, 2014-08-14 22:22:04]

https://codereview.chromium.org/476553002/diff/40001/Source/core/dom/Document...
File Source/core/dom/Document.cpp (right):

https://codereview.chromium.org/476553002/diff/40001/Source/core/dom/Document...
Source/core/dom/Document.cpp:5644:
element->setInlineStyleProperty(CSSPropertyVisibility, CSSValueHidden);
What if a descendant has visibility: visible set explicitly?

[oystein (OOO til 10th of July), 2014-08-14 22:33:14]

https://codereview.chromium.org/476553002/diff/40001/Source/core/dom/Document...
File Source/core/dom/Document.cpp (right):

https://codereview.chromium.org/476553002/diff/40001/Source/core/dom/Document...
Source/core/dom/Document.cpp:5644:
element->setInlineStyleProperty(CSSPropertyVisibility, CSSValueHidden);
On 2014/08/14 22:22:04, abarth wrote:
> What if a descendant has visibility: visible set explicitly?

Hm yeah I see what you mean. What do you suggest doing in this case? 

I could see either
1) Leaving it as it is and interpreting that explicit value as meaning the
content creator doesn't want the element hidden when the transition is happening
(i.e. get duplicated rather than "moved"), or
2) Recurse and set visibility: hidden on all children.
3) ?

[abarth-chromium, 2014-08-14 22:43:48]

What's the purpose of hiding the transition elements?

[oystein (OOO til 10th of July), 2014-08-14 22:46:54]

On 2014/08/14 22:43:48, abarth wrote:
> What's the purpose of hiding the transition elements?

Once we deserialize them into the transition layer, we effectively have two
duplicates on top of one another. The idea is to hide the original before we
apply any CSS anim to the copy, which could cause repositioning and visibly
expose the original.

[abarth-chromium, 2014-08-14 22:50:32]

On 2014/08/14 at 22:46:54, oysteine wrote:
> On 2014/08/14 22:43:48, abarth wrote:
> > What's the purpose of hiding the transition elements?
> 
> Once we deserialize them into the transition layer, we effectively have two
duplicates on top of one another. The idea is to hide the original before we
apply any CSS anim to the copy, which could cause repositioning and visibly
expose the original.

I see.  Of course lifting the element to the transition layer will break paint
ordering, but presumably you're ok with that.  I guess you have some way of
making sure the elements are visible in the transition layer before hiding the
original ones...

Maybe the easiest thing to do would be to set the opacity of the original
elements to zero.  That will have the effect of making them invisible without
having (much) effect on layout.  Technically it will cause the layout of the
page to change in some obscure ways, but in most cases it won't change any
pixels.

[oystein (OOO til 10th of July), 2014-08-14 22:57:43]

On 2014/08/14 22:50:32, abarth wrote:
> On 2014/08/14 at 22:46:54, oysteine wrote:
> > On 2014/08/14 22:43:48, abarth wrote:
> > > What's the purpose of hiding the transition elements?
> > 
> > Once we deserialize them into the transition layer, we effectively have two
> duplicates on top of one another. The idea is to hide the original before we
> apply any CSS anim to the copy, which could cause repositioning and visibly
> expose the original.
> 
> I see.  Of course lifting the element to the transition layer will break paint
> ordering, but presumably you're ok with that.  I guess you have some way of
> making sure the elements are visible in the transition layer before hiding the
> original ones...

There's some logic on the embedder-side of things which delays this call until
the transition page says its ready, though right now it doesn't fully wait for
image loads and is something we'll have to iterate on (i.e. whether we should
wait for the full OnLoad or have some sort of timeout).

> Maybe the easiest thing to do would be to set the opacity of the original
> elements to zero.  That will have the effect of making them invisible without
> having (much) effect on layout.  Technically it will cause the layout of the
> page to change in some obscure ways, but in most cases it won't change any
> pixels.

Great, thanks. Done!

[abarth-chromium, 2014-08-14 23:01:43]

LGTM

[oystein (OOO til 10th of July), 2014-08-14 23:03:32]

The CQ bit was checked by oysteine@chromium.org

[commit-bot: I haz the power, 2014-08-14 23:04:49]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/oysteine@chromium.org/476553002/60001

[commit-bot: I haz the power, 2014-08-14 23:46:27]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  linux_blink_dbg on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/linux_blink_dbg/builds/2...)
  linux_blink_rel on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/linux_blink_rel/builds/2...)
  mac_blink_rel on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/mac_blink_rel/builds/19894)
  win_blink_rel on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/win_blink_rel/builds/22844)

[commit-bot: I haz the power, 2014-08-14 23:59:12]

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

[commit-bot: I haz the power, 2014-08-14 23:59:13]

Try jobs failed on following builders:
  mac_blink_rel on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/mac_blink_rel/builds/19898)

[oystein (OOO til 10th of July), 2014-08-15 00:44:02]

The CQ bit was checked by oysteine@chromium.org

[commit-bot: I haz the power, 2014-08-15 00:45:21]

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

[commit-bot: I haz the power, 2014-08-15 02:21:53]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  linux_blink_rel on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/linux_blink_rel/builds/2...)

[commit-bot: I haz the power, 2014-08-15 02:57:50]

Committed patchset #3 (80001) as 180318