1029285 - Use flexbox and viewport units to simplify ImageDocument's implementation

Status: NEW

(Core :: Layout: Images, Video, and HTML Frames, defect)

Description

[Seth Fowler [:seth] [:s2h]]

Right now to implement centering and shrink-to-fit, ImageDocument relies on a combination of absolute positioning and manually setting the width and height of the image according to computations performed in C++.

We don't need to do this anymore. With flexbox (which allows auto margins to work vertically as well as horizontally) and viewport units (which enable us to implement shrink-to-fit in CSS), we can move a significant amount of C++ code into CSS. What's really nice is this will also resolve a long-standing bug involving image-orientation (bug 997604).

Comment 1

[Seth Fowler [:seth] [:s2h]]

Attachment: (Part 1) - Handle shrink-to-fit for ImageDocument in CSS, using flexbox and viewport units

The CSS component of this patch. See comment 0 for the basic idea.

Comment 2

[Seth Fowler [:seth] [:s2h]]

Attachment: (Part 2) - Simplify ImageDocument, relying more on layout

This is the C++ portion of the patch. The main things that change here are:

(1) We rely on CSS and layout to implement shrink-to-zoom. We no longer do any manual setting of the image's width or height. We only use those values now for the title and for the computations in ScrollImageTo. This caused many small changes throughout the ImageDocument code.

(2) The state machine that ImageDocument uses is made more explicit. This also caused many small changes throughout the code, and it enabled us to reduce the amount of state that ImageDocument has to keep.

The combination of (1) and (2) would actually allow us to remove even more code if we also made changes to nsIImageDocument, but I decided to leave such changes out of this bug since nsIImageDocument is exposed to addons.

Comment 3

[Seth Fowler [:seth] [:s2h]]

Note, by the way, that these patches do not resolve bug 997604 by themselves. To fully fix it requires that we also land the remaining patch in bug 997604 as well as bug 997010.

Comment 4

[Olli Pettay [:smaug][[e-mail]]]

Curious, did you test page zoom with these changes? With and without shrink-to-fit.

Comment 5

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8445655 [details] [diff] [review]
(Part 2) - Simplify ImageDocument, relying more on layout

> nsresult
> ImageDocument::CreateSyntheticDocument()
> {
>   // Synthesize an html document that refers to the image
>   nsresult rv = MediaDocument::CreateSyntheticDocument();
>   NS_ENSURE_SUCCESS(rv, rv);
> 
>   // Add the image element
>   Element* body = GetBodyElement();
>   if (!body) {
>     NS_WARNING("no body on image document!");
>     return NS_ERROR_FAILURE;
>   }
> 
>   nsCOMPtr<nsINodeInfo> nodeInfo;
>+  nodeInfo = mNodeInfoManager->GetNodeInfo(nsGkAtoms::div, nullptr,
>+                                           kNameSpaceID_XHTML,
>+                                           nsIDOMNode::ELEMENT_NODE);
>+  mContainerDivContent = NS_NewHTMLDivElement(nodeInfo.forget());
>+  if (!mContainerDivContent) {
>+    return NS_ERROR_OUT_OF_MEMORY;
>+  }
>+  mContainerDivContent->SetAttr(kNameSpaceID_None, nsGkAtoms::id, NS_LITERAL_STRING("container"), false);
>+
>+  nodeInfo = mNodeInfoManager->GetNodeInfo(nsGkAtoms::div, nullptr,
>+                                           kNameSpaceID_XHTML,
>+                                           nsIDOMNode::ELEMENT_NODE);
>+  nsRefPtr<nsGenericHTMLElement> itemDiv =
>+    NS_NewHTMLDivElement(nodeInfo.forget());
>+  if (!itemDiv) {
>+    return NS_ERROR_OUT_OF_MEMORY;
>+  }
>+  itemDiv->SetAttr(kNameSpaceID_None, nsGkAtoms::id, NS_LITERAL_STRING("item"), false);
>+
>   nodeInfo = mNodeInfoManager->GetNodeInfo(nsGkAtoms::img, nullptr,
>                                            kNameSpaceID_XHTML,
>                                            nsIDOMNode::ELEMENT_NODE);
> 
>   mImageContent = NS_NewHTMLImageElement(nodeInfo.forget());
>   if (!mImageContent) {
>     return NS_ERROR_OUT_OF_MEMORY;
>   }
>+
>   nsCOMPtr<nsIImageLoadingContent> imageLoader = do_QueryInterface(mImageContent);
>   NS_ENSURE_TRUE(imageLoader, NS_ERROR_UNEXPECTED);
> 
>   nsAutoCString src;
>   mDocumentURI->GetSpec(src);
> 
>   NS_ConvertUTF8toUTF16 srcString(src);
>   // Make sure not to start the image load from here...
>   imageLoader->SetLoadingEnabled(false);
>   mImageContent->SetAttr(kNameSpaceID_None, nsGkAtoms::src, srcString, false);
>   mImageContent->SetAttr(kNameSpaceID_None, nsGkAtoms::alt, srcString, false);
> 
>-  body->AppendChildTo(mImageContent, false);
>+  itemDiv->AppendChildTo(mImageContent, false);
>+  mContainerDivContent->AppendChildTo(itemDiv, false);
>+  body->AppendChildTo(mContainerDivContent, false);
This stuff is against the spec
http://www.whatwg.org/specs/web-apps/current-work/multipage/history.html#read-media
and would very likely cause regressions.

Comment 6

[Seth Fowler [:seth] [:s2h]]

(In reply to Olli Pettay [:smaug] from comment #5)
> This stuff is against the spec
> http://www.whatwg.org/specs/web-apps/current-work/multipage/history.
> html#read-media
> and would very likely cause regressions.

What regressions would this cause?

Comment 7

[Seth Fowler [:seth] [:s2h]]

To be clear, this change does *not* violate the spec, by my reading, because the spec says that the user agent "should" create the DOM structure described there, not that it "must". That means we are free to use an approach other than the one described there as long as we consider any possible problems that using a different approach might cause.

So, what kind of regression do you think this could cause? As far as I know, this could only conceivably cause a problem for addons that make assumptions about the exact structure of the DOM for image documents.

Comment 8

[Olli Pettay [:smaug][[e-mail]]]

A web page could have an iframe and load image there, and then modify the DOM of that iframe.

Comment 9

[Seth Fowler [:seth] [:s2h]]

That does seem like something that could cause a regression, and I agree that it'd be ideal to avoid it if we can.

Fortunately, it turns out that we should be able to. It turns out that the intermediate div arrangement I'm using in the current version of the code is only necessary because of a bug in our current implementation of flexbox. dholbert's investigating the issue. (As of this writing, the bug hasn't been filed, but I'll add it as a blocker for this bug once it does get filed.)

Once that bug is fixed, we should be able to maintain the same DOM structure that we have now, by making the body element the flex container and making the img element a flex item directly.

I'm going to unset review flags until the flexbox bug gets fixed, since both the CSS and the C++ code will need to change.

Comment 10

[[e-mail]]

Comment on attachment 8445652 [details] [diff] [review]
(Part 1) - Handle shrink-to-fit for ImageDocument in CSS, using flexbox and viewport units

Why have you moved the CSS for top level image documents out of TopLevelImageDocument.css and into ImageDocument.css?

Comment 11

[Seth Fowler [:seth] [:s2h]]

(In reply to [e-mail] from comment #10)
> Comment on attachment 8445652 [details] [diff] [review]
> (Part 1) - Handle shrink-to-fit for ImageDocument in CSS, using flexbox and
> viewport units
> 
> Why have you moved the CSS for top level image documents out of
> TopLevelImageDocument.css and into ImageDocument.css?

I've only moved one property, |margin: 0| for the body element. This makes our behavior the same as Blink and Webkit, so there's no web compatibility problem. It looks better even with the old CSS.

The properties we were applying for img elements in TopLevelImageDocument.css are no longer relevant. They haven't been moved, they've been totally removed.

Comment 12

[[e-mail]]

(In reply to Seth Fowler from comment #11)
> The properties we were applying for img elements in
> TopLevelImageDocument.css are no longer relevant.

No longer relevant to what? Their purpose was solely to centre only top level image documents.

Comment 13

[Seth Fowler [:seth] [:s2h]]

Neil and I discussed this in more detail on IRC and concluded that we want to maintain the current behavior of not centering non-top-level image documents.

Currently with WebKit/Blink, non-top-level image documents have the following properties:

- No margin
- Not centered
- No click-to-fit

Before this patch, in Firefox non-top-level image documents have the following properties:

- Non-zero margin
- Not centered
- Click-to-fit enabled

After this patch, Firefox non-top-level image documents will have the following properties:

- Zero margin
- Not centered
- Click-to-fit enabled

2 out of 3 ain't bad. =) This gets us closer to the same behavior.

Comment 14

[BugBot [:suhaib / :marco/ :calixte]]

The bug assignee didn't login in Bugzilla in the last 7 months, so the assignee is being reset.