757859 - Add a getHeight/getWidth/onResize methods to mozbrowser

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect)

Description

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

There is at least 2 use cases where it could be useful to know the width/height of the content of an iframe without accessing it directly. Having a callback/event when the frame is resize too would be perfect.

The use cases is for the keyboard. The keyboard is inside an iframe and it could be nice to change the size of the iframe depending on the content of the keyboard.

The other use case will be for managing 'disposition:inline' activities. It will simply make it works.

Comment 1

[Ben Francis [:benfrancis]]

Vivien: I've added these to the browser API requirements on the wiki https://wiki.mozilla.org/WebAPI/BrowserAPI - can you take a look and fill in any design details you think are missing? This API is going to be reviewed by the security team soon and I want to make sure this wiki page is as complete as possible.

BTW, do you think these are requirements for mozbrowser, mozapp, or both?

Dietrich: I'm not sure how to prioritise these new requirements relative to the rest of the browser API requirements on this page, can you help here?

Comment 2

[alberto.pastor]

Ben: There is another issue with the keyboard that I think could be added to BrowserAPi as well. As the content of the iframe can't overflow, when an alternate keyboard for a key is shown and it overflows the iframe height, it's not shown completely. To fix this, we are assigning 100% height to the keyboard's iframe, but it conflicts with the currentApp touch events.

Make it sense for you to add some property allowing a "iframe seamless"? Another alternatives?

https://github.com/andreasgal/gaia/pull/1464#r873126

Thanks!

Comment 3

[Justin Lebar (not reading bugmail)]

> This API is going to be reviewed by the security team soon and I want to make sure this 
> wiki page is as complete as possible.

fwiw I think we're clearly still in the early stages of this API.  As evidence, I submit the fact that we've filed this bug and bug 710231 in the past few days, when nobody had thought about either of these cases before.  I think it's likely that we'll have lots more additions similarly unplanned additions to this API.

Thinking harder about the API isn't going to help; we just need to use it and see what we need.

Comment 4

[Ben Francis [:benfrancis]]

I'm wondering if these features (and the one Alberto mentions) should be part of mozapp, but not mozbrowser? Are there any use cases for these features when building a browser?

Comment 5

[Dale Harvey (:daleharvey)]

I think we need to consider mozapp and mozbrowser pretty much orthogonal when it comes to window management. They will both want the same functionality available (even if the current UX specs of the browser doesnt require it, someone may want to write a browser that looks very much like our window manager)

Because communication over the process boundary is async, these will probably be better implemented as getDimensions or getInfo, as we dont to have to write

> getWidth().onsuccess = function() { 
>   getHeight().onsuccess = function() { 
>     ...

Happy for input on this, it will be fairly quick to write, both this and contextmenu patches will be in for tommorrow night

Comment 6

[Dale Harvey (:daleharvey)]

Attachment: Add getDimensions property and resize event to mozbrowser

Also added the dimensions details to the resize event which seemed sensible

Comment 7

[Ben Francis [:benfrancis]]

Cool. Feel free to edit https://wiki.mozilla.org/WebAPI/BrowserAPI with any changed design details (i.e. getDimensions instead of getHeight and getWidth) and use cases.

Comment 8

[Justin Lebar (not reading bugmail)]

Comment on attachment 628149 [details] [diff] [review]
Add getDimensions property and resize event to mozbrowser

This looks great.  Just a few changes:

 * Can we call this getInnerDimensions (like window.inner{Height,Width})?  Or perhaps getContentDimensions (like iframe.contentWindow)?

 * Can you please test that what's being returned is the width/height of the actual content by loading into the iframe a page with height and width larger than the iframe itself?

 * Can you please test that we get resize events in the following cases:

    ** width of iframe changes and content's natural width < iframe's width (what you currently have)
    ** height/width of content changes (and content's width > iframe's width)
    ** we load a new content page with height/width different from the previous content page.

Comment 9

[Dale Harvey (:daleharvey)]

This is my bad, I didnt think about the use case properly

Windows dont fire resize when their content changes, Mutation Observers / Events might be useful? however Events are extremely slow, and Observers I am not sure about their status, will test

Comment 10

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

Note, MutationObserver/Events are just for changes in the DOM tree. Nothing more nothing less.

Comment 11

[Dale Harvey (:daleharvey)]

Yup, orientation changes and general window changes are covered by the resize event, page load events I think its safe to unconditionally fire a resized event onloadend, 

However when the page changes the size of its own content, the only think I can think of is polling, which is how most X.resize plugins work, or mutation events which as long as we can observe 'all' mutations (meaning style.width = '900' etc) should cover anything that the page can do to change its own size (of course not ever mutation will change size) 

Both of those are pretty ugly though, is there any other alternatives, Vivien did you have any ideas in mind when thinking about the API?

Comment 12

[Dale Harvey (:daleharvey)]

Attachment: Added tests based on review (doesnt pass)

So I have renamed to getContentSize, and updated the tests, now we just need to figure out the best strategy for determining when the content of a page has resized

Comment 13

[Dale Harvey (:daleharvey)]

Comment on attachment 628839 [details] [diff] [review]
Added tests based on review (doesnt pass)

Review of attachment 628839 [details] [diff] [review]:
-----------------------------------------------------------------

Blocked since I cant think of a sensible way to detect the content of an iframe resize, any ideas?

Comment 14

[Justin Lebar (not reading bugmail)]

bz, do we have a way to detect when an iframe's content height changes?  (We don't really care about the content width, afaict.)

Comment 15

[Boris Zbarsky [:bzbarsky]]

> bz, do we have a way to detect when an iframe's content height changes? 

Can we define "content height" (while keeping in mind positioned, floated, transformed, etc stuff)?

In any case the answer is almost certainly "probably not, but we could add something".  But I'd like to understand what changes we're trying to detect first.

Comment 16

[Dale Harvey (:daleharvey)]

vingetun would need to specify, but I believe its anything that would trigger schollHeight changes / force a scrollbar to show

Also got pointed to 
https://developer.mozilla.org/en/DOM/Detecting_document_width_and_height_changes
which I havent tested yet

Comment 17

[Justin Lebar (not reading bugmail)]

> Can we define "content height" (while keeping in mind positioned, floated, transformed, etc stuff)?

Surely there's some notion of "what's the minimum-sized frame necessary to contain this thing without showing scrollbars"?

Vivien can comment more on the use-case, but aiui, we have a keyboard which has arbitrary height.  The keyboard is inside an iframe mozbrowser.  We want to resize the frame above the keyboard (also an iframe mozbrowser) so that the whole keyboard is shown.

Perhaps for this use-case we should just make the keyboard tell its parent how big it "wants" to be.

Comment 18

[Boris Zbarsky [:bzbarsky]]

> Surely there's some notion of "what's the minimum-sized frame necessary to contain
> this thing without showing scrollbars"?

Consider this HTML document:

  <!DOCTYPE html>
  <html style="height: 100%">
    <body style="height: 200%; border: 10px solid red"></body>
  </html>

That will always have a vertical scrollbar.

But for non-pathological cases like the one described in comment 17, the event from comment 16 might work.

Comment 19

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

Comment on attachment 628839 [details] [diff] [review]
Added tests based on review (doesnt pass)

Removing feedback until a new version with MozScrolledAreaChange.

Comment 20

[Dale Harvey (:daleharvey)]

Attachment: Bug 757859 - Add getContentDimensions and mozscrollareachanged event to mozbrowser. r=jlebar

Tests pass now

Renamed the event to mozbrowserareascrolled, it seems sensible to take the name of the existing event.

I removed out the resize listener as it seemed pointless, only the parent can change the width

Comment 21

[Justin Lebar (not reading bugmail)]

Did you mean to flag someone in particular for feedback here?

Comment 22

[Dale Harvey (:daleharvey)]

Sorry I am sure I filled that in, but yeh figured vivien should take a look over it before you

Comment 23

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

Comment on attachment 645485 [details] [diff] [review]
Bug 757859 - Add getContentDimensions and mozscrollareachanged event to mozbrowser. r=jlebar

Review of attachment 645485 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/browser-element/BrowserElementChild.js
@@ +553,5 @@
> +    return {
> +      width: content.document.body.offsetWidth,
> +      height: content.document.body.offsetHeight
> +    }
> +  },

Why don't you returns the same set of values from width/height/x/y for both methods?

Comment 24

[Dale Harvey (:daleharvey)]

Vivien, as far as I remember this wasnt needed anymore, shall I close?

Comment 25

[Chris Lord [:cwiiis]]

Reopening this, as we need it for bug 1039519.

Specifically, we need to know when the contents of a browser iframe has changed size, and we need to be able to query that size, which I believe is all that this bug covers.

Comment 26

[Chris Lord [:cwiiis]]

Dale very graciously agreed to finish this off on IRC :)

Comment 27

[Dale Harvey (:daleharvey)]

Attachment: 757859.patch

Reviving a 2 year old patch :)

|let clientObj = Cu.cloneInto(data.json.successRv, this._window);| was needed otherwise we couldnt access data within objects from content, seems like it should work with the other calls.

Comment 28

[Dale Harvey (:daleharvey)]

Comment on attachment 8460695 [details] [diff] [review]
757859.patch

Chris, would also like some feedback, in particular about which exact information you will need, getContentDimensions is offsetHeight so the dimensions of the content, and the width / height from the scrollareachanged are scrollWidth / scrollHeight, so the viewport.

Comment 29

[Chris Lord [:cwiiis]]

(In reply to Dale Harvey (:daleharvey) from comment #28)
> Comment on attachment 8460695 [details] [diff] [review]
> 757859.patch
> 
> Chris, would also like some feedback, in particular about which exact
> information you will need, getContentDimensions is offsetHeight so the
> dimensions of the content, and the width / height from the scrollareachanged
> are scrollWidth / scrollHeight, so the viewport.

I'd like the size of the viewport, so scrollWidth/scrollHeight. I'm not sure when these values would differ in practice though?

Comment 30

[Dale Harvey (:daleharvey)]

Comment on attachment 8460695 [details] [diff] [review]
757859.patch

Clearing review for now and going to wait until we are sure the reported values are the ones we need

Comment 31

[Dale Harvey (:daleharvey)]

Comment on attachment 8460695 [details] [diff] [review]
757859.patch

So I can keep track of this, Chris can you report back f? when you test locally and are fairly certain that this does / does not report the values you need for the scroll functionality, then I can get this through review asap and not block that stuff landing.

Cheers

Comment 32

[Chris Lord [:cwiiis]]

(In reply to Dale Harvey (:daleharvey) from comment #31)
> Comment on attachment 8460695 [details] [diff] [review]
> 757859.patch
> 
> So I can keep track of this, Chris can you report back f? when you test
> locally and are fairly certain that this does / does not report the values
> you need for the scroll functionality, then I can get this through review
> asap and not block that stuff landing.
> 
> Cheers

I'm currently blocked on this work, but I'll report back as soon as I can - sorry about that.

Comment 33

[Dale Harvey (:daleharvey)]

Its no bother at all just wanted to make sure I was on top of landing this when you need it

Comment 34

[Dale Harvey (:daleharvey)]

closing and clearing needinfo again as it doesnt look like this is needed as yet

Comment 35

[Chris Lord [:cwiiis]]

Reopening as we will need this, I've just been too lax and haven't filed the bug yet... Will do so now.

Comment 36

[Kevin Grandon :kgrandon]

Chris - I think Dale has a *lot* of things assigned to him right now. Do you think you could drive this patch to landing?

Comment 37

[Chris Lord [:cwiiis]]

(In reply to Kevin Grandon :kgrandon from comment #36)
> Chris - I think Dale has a *lot* of things assigned to him right now. Do you
> think you could drive this patch to landing?

yup, no problem - makes sense, Dale's done the work already.

Comment 38

[Chris Lord [:cwiiis]]

Attachment: Add get-contentdimensions and scrollareachanged events to browser element

This is Dale's patch, modified very slightly to do what we need. Seems to work in testing.

Comment 39

[Kevin Grandon :kgrandon]

Adding checkin-needed since Chris might be on PTO already.

Comment 40

[Carsten Book [:Tomcat]]

(In reply to Kevin Grandon :kgrandon from comment #39)
> Adding checkin-needed since Chris might be on PTO already.

hey Kevin do you have a try link (to check nothing breaks with this patch) ?

Comment 41

[Kevin Grandon :kgrandon]

Thanks for the reminder. Pushed to try here: https://tbpl.mozilla.org/?tree=Try&rev=22a78427dacb

Comment 42

[Kevin Grandon :kgrandon]

Try was fully red for some reason, so trying again: https://tbpl.mozilla.org/?tree=Try&rev=4c7a92850020

Comment 43

[Kevin Grandon :kgrandon]

Proper try link now, did not apply the patch correctly before :-/

https://tbpl.mozilla.org/?tree=Try&rev=283092bf68ef

Comment 44

[Kevin Grandon :kgrandon]

Clearing CC on :Tomcat because we pulled him in here with the checkin-needed.

Seems we might have some failures to clean-up before this lands: https://tbpl.mozilla.org/php/getParsedLog.php?id=46984970&tree=Try

Chris or Dale - please take a look if you have time.

Comment 45

[Chris Lord [:cwiiis]]

So reapplying this now, it seems something has changed in Gecko and the browser element is no longer getting the size-changed event... So the test failures are legitimate. I'll try to figure out what's happened...

Comment 46

[Chris Lord [:cwiiis]]

Whoops, no, I take that back, that's just my clumsy debugging output breaking things... Will investigate the tests.

Comment 47

[Chris Lord [:cwiiis]]

Attachment: Add get-contentdimensions and scrollareachanged events to browser element

Just a '<' instead of '<=' :p Carrying r+, will push to inbound. Tests pass locally.

Comment 48

[Chris Lord [:cwiiis]]

Pushed to inbound: https://hg.mozilla.org/integration/mozilla-inbound/rev/0f89f564efdb

Comment 49

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/0f89f564efdb

Comment 50

[Chris Lord [:cwiiis]]

Comment on attachment 8483526 [details] [diff] [review]
Add get-contentdimensions and scrollareachanged events to browser element

Approval Request Comment
[Feature/regressing bug #]: Ability to know when a page in a browser iframe changes size (required for a b2g 2.1 blocker)
[User impact if declined]: Pages will always be able to scroll, even short pages that shouldn't need to.
[Describe test coverage new/current, TBPL]: Covered by new mochitests
[Risks and why]: Low risk, just an extra feature added to the browser element
[String/UUID change made/needed]: None

Comment 51

[Chris Lord [:cwiiis]]

[Blocking Requested - why for this release]: This blocks a 2.1 blocker, requesting the blocker flag on this in the hope it speeds up the approval process.

Comment 52

[bhavana bajaj [:bajaj]]

(In reply to Chris Lord [:cwiiis] from comment #51)
> [Blocking Requested - why for this release]: This blocks a 2.1 blocker,
> requesting the blocker flag on this in the hope it speeds up the approval
> process.

Blocking on it only because it blocks a blocker else I would have cleared the blocking nom and just acted on approval here.

Comment 53

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/a509add936a5

Comment 54

[Jeremiah Lee]

Cannot verify due to no STRs. This issue seems to be back-end issue.