Add a getHeight/getWidth/onResize methods to mozbrowser

RESOLVED FIXED in Firefox 34

Status

()

defect
RESOLVED FIXED
7 years ago
4 years ago

People

(Reporter: vingtetun, Assigned: cwiiis)

Tracking

(Blocks 1 bug, {dev-doc-needed})

Trunk
mozilla35
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(blocking-b2g:2.2+, firefox33 wontfix, firefox34 fixed, firefox35 fixed, b2g-v2.1 fixed, b2g-v2.2 fixed)

Details

(Whiteboard: [systemsfe][tako])

Attachments

(1 attachment, 5 obsolete attachments)

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.
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

7 years ago
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!
> 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.
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?
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
Also added the dimensions details to the resize event which seemed sensible
Assignee: nobody → dale
Status: NEW → ASSIGNED
Attachment #628149 - Flags: review?(justin.lebar+bug)
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 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.
Attachment #628149 - Flags: review?(justin.lebar+bug) → review+
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
Note, MutationObserver/Events are just for changes in the DOM tree. Nothing more nothing less.
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?
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
Attachment #628149 - Attachment is obsolete: true
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?
Attachment #628839 - Flags: feedback?(21)
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.)
> 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.
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
> 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.
> 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 on attachment 628839 [details] [diff] [review]
Added tests based on review (doesnt pass)

Removing feedback until a new version with MozScrolledAreaChange.
Attachment #628839 - Flags: feedback?(21)
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
Attachment #628839 - Attachment is obsolete: true
Attachment #645485 - Flags: feedback?
Did you mean to flag someone in particular for feedback here?
Attachment #645485 - Flags: feedback? → feedback?(21)
Sorry I am sure I filled that in, but yeh figured vivien should take a look over it before you
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?
Vivien, as far as I remember this wasnt needed anymore, shall I close?
Status: ASSIGNED → RESOLVED
Last Resolved: 7 years ago
Resolution: --- → INVALID
Attachment #645485 - Flags: feedback?(21)
(Assignee)

Comment 25

5 years ago
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.
Assignee: dale → nobody
Status: RESOLVED → REOPENED
Resolution: INVALID → ---
(Assignee)

Updated

5 years ago
Blocks: 1039519
(Assignee)

Comment 26

5 years ago
Dale very graciously agreed to finish this off on IRC :)
Assignee: nobody → dale
Posted patch 757859.patch (obsolete) — Splinter Review
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.
Attachment #645485 - Attachment is obsolete: true
Attachment #8460695 - Flags: review?(ehsan)
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.
Attachment #8460695 - Flags: feedback?(chrislord.net)
(Assignee)

Comment 29

5 years ago
(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?
(Assignee)

Updated

5 years ago
Attachment #8460695 - Flags: feedback?(chrislord.net) → feedback+
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
Attachment #8460695 - Flags: review?(ehsan)
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
Attachment #8460695 - Flags: feedback+ → feedback?(chrislord.net)
Whiteboard: [systemsfe][tako]
Target Milestone: --- → 2.1 S1 (1aug)
(Assignee)

Comment 32

5 years ago
(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.
Its no bother at all just wanted to make sure I was on top of landing this when you need it
Target Milestone: 2.1 S1 (1aug) → 2.1 S2 (15aug)
closing and clearing needinfo again as it doesnt look like this is needed as yet
Status: REOPENED → RESOLVED
Last Resolved: 7 years ago5 years ago
Resolution: --- → INVALID
Attachment #8460695 - Flags: feedback?(chrislord.net)
(Assignee)

Comment 35

5 years ago
Reopening as we will need this, I've just been too lax and haven't filed the bug yet... Will do so now.
Status: RESOLVED → REOPENED
Resolution: INVALID → ---
(Assignee)

Updated

5 years ago
No longer blocks: 1039519
(Assignee)

Updated

5 years ago
Blocks: 1058637
Chris - I think Dale has a *lot* of things assigned to him right now. Do you think you could drive this patch to landing?
Flags: needinfo?(chrislord.net)
(Assignee)

Comment 37

5 years ago
(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.
Assignee: dale → chrislord.net
Flags: needinfo?(chrislord.net)
(Assignee)

Comment 38

5 years ago
This is Dale's patch, modified very slightly to do what we need. Seems to work in testing.
Attachment #8460695 - Attachment is obsolete: true
Attachment #8480043 - Flags: review?(ehsan)

Updated

5 years ago
Attachment #8480043 - Flags: review?(ehsan) → review+
Adding checkin-needed since Chris might be on PTO already.
Keywords: checkin-needed
(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) ?
Flags: needinfo?(kgrandon)
Keywords: checkin-needed
Comment hidden (obsolete)
Comment hidden (obsolete)
Proper try link now, did not apply the patch correctly before :-/

https://tbpl.mozilla.org/?tree=Try&rev=283092bf68ef
Target Milestone: 2.1 S2 (15aug) → 2.1 S3 (29aug)
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.
Status: REOPENED → ASSIGNED
(Assignee)

Comment 45

5 years ago
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...
(Assignee)

Comment 46

5 years ago
Whoops, no, I take that back, that's just my clumsy debugging output breaking things... Will investigate the tests.
(Assignee)

Comment 47

5 years ago
Just a '<' instead of '<=' :p Carrying r+, will push to inbound. Tests pass locally.
Attachment #8480043 - Attachment is obsolete: true
Attachment #8483526 - Flags: review+
https://hg.mozilla.org/mozilla-central/rev/0f89f564efdb
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago5 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: 2.1 S3 (29aug) → mozilla35
(Assignee)

Comment 50

5 years ago
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
Attachment #8483526 - Flags: approval-mozilla-aurora?
(Assignee)

Comment 51

5 years ago
[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-b2g: --- → 2.1?
(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.
blocking-b2g: 2.1? → 2.2+
Attachment #8483526 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+

Updated

5 years ago
Keywords: dev-doc-needed

Comment 54

5 years ago
Cannot verify due to no STRs. This issue seems to be back-end issue.
QA Whiteboard: [QAnalyst-Triage?] [QAnalyst-verify-]
Flags: needinfo?(ktucker)
QA Whiteboard: [QAnalyst-Triage?] [QAnalyst-verify-] → [QAnalyst-Triage+] [QAnalyst-verify-]
Flags: needinfo?(ktucker)
(Assignee)

Updated

4 years ago
Depends on: 1170644
You need to log in before you can comment on or make changes to this bug.