Opened 6 years ago

Closed 6 years ago

Last modified 6 years ago

#17084 closed defect (fixed)

[Patch CLA]ScrollablePane: "height=inherit" miscalculated

Reported by: Paul Christopher Owned by: Patrick Ruzand
Priority: undecided Milestone: 1.9.1
Component: DojoX Mobile Version: 1.9.0rc2
Keywords: Cc:
Blocked By: Blocking:

Description

See attached test case: "height:inherit" seems to be not really usable with ScrollablePane, since scrollable.js replaces the height with "this.domNode.offsetParent.offsetHeight + "px";" (which is actually the body's height), and not just the real parent's height (which is "this.domNode.parentNode.offsetHeight + 'px';").

Attachments (3)

[CLA]ScrollablePane.js.diff (493 bytes) - added by Paul Christopher 6 years ago.
test_ScrollablePane.html (6.2 KB) - added by Paul Christopher 6 years ago.
test_ScrollablePane2.html (6.2 KB) - added by Sebastien Brunot 6 years ago.
using position:relative for the sizing DIVs

Download all attachments as: .zip

Change History (25)

Changed 6 years ago by Paul Christopher

Attachment: [CLA]ScrollablePane.js.diff added

Changed 6 years ago by Paul Christopher

Attachment: test_ScrollablePane.html added

comment:1 Changed 6 years ago by Sebastien Brunot

Could you check if the patch http://trac.dojotoolkit.org/attachment/ticket/17050/tickets17050-15761.patch (for both tickets #17050 and #15761) does not already solve the issue ?

comment:2 Changed 6 years ago by Paul Christopher

Salut Sébastien! I have just checked out the latest dojox/mobile version from the SVN repository, have applied your patch, and - alas - the issue still exists. I think you are addressing different bug with your patch. Just run my test case and I think you'll see what I mean.

comment:3 Changed 6 years ago by Sebastien Brunot

Hi,

I now remember that I ran into the same issue while writing some tests... As the offsetParent is used for all calculations, the position of any container div added around a scrollable should be set to "relative". I've attached a new version of your test page as a sample (test_ScrollablePane2.html).

Changed 6 years ago by Sebastien Brunot

Attachment: test_ScrollablePane2.html added

using position:relative for the sizing DIVs

comment:4 Changed 6 years ago by Sebastien Brunot

Maybe adding this to the documentation of scrollable.inherit would be enough... What do you think ?

comment:5 Changed 6 years ago by Paul Christopher

Ahhh, I see. The wrapping div should not be set to "position:static". It needs to be either "position:relative", "position:absolute" or "position:fixed" so as to mark it as the !offsetParent. Yes, I think a hint in the documentation is enough but necessary: At least I would have not been able to come up with a working solution without your help. And I guess other people will run into the same issue.

comment:6 Changed 6 years ago by Sebastien Brunot

Documentation has been updated for the upcoming version of dojo: http://livedocs.dojotoolkit.org/dojox/mobile/ScrollablePane

comment:7 Changed 6 years ago by Eric Durocher

Maybe you could also add a link to the new section in the description of the "height" property? Like:

If "inherit" is specified, the height is inherited from its offset parent (see <link to Inheriting height...>)

comment:8 Changed 6 years ago by Sebastien Brunot

Pull request created to update documentation for 1.9.1: https://github.com/dojo/docs/pull/87

comment:9 Changed 6 years ago by Sebastien Brunot

Link added to the description of the "height" property.

comment:10 Changed 6 years ago by Sebastien Brunot

The pull request has been accepted and merged for 1.9.1: I think that we can close this ticket.

comment:11 Changed 6 years ago by Patrick Ruzand

Owner: changed from Eric Durocher to Patrick Ruzand
Status: newassigned

comment:12 Changed 6 years ago by Adrian Vasiliu

Doesn't the same hold for ScrollableView?? Shouldn't its ref. doc be improved similarly? If so, can you create a PR for it too?

comment:13 Changed 6 years ago by Adrian Vasiliu

@sbrunot, any news about it? (see the comment above).

comment:14 Changed 6 years ago by Sebastien Brunot

The documentation for ScrollableView? is already up to date: If “inherit” is specified, the height is inherited from its offset parent.

Do you have a specific inheritance use case for which you would like to see an example added to the ScrollableView? documentation ?

comment:15 Changed 6 years ago by Adrian Vasiliu

You have added a section called "Inheriting height from a parent div" to the ref. doc. of ScrollablePane?, and this section does not exist in the doc of ScrollableView?. My question is: is there a reason a reader of the doc of ScrollableView? shouldn't find the same information? If there is, fine. If there isn't, either this section should be duplicated, or, better, could exist for one module and be referenced from the doc of the other.

Last edited 6 years ago by Adrian Vasiliu (previous) (diff)

comment:16 Changed 6 years ago by Sebastien Brunot

The section "Inheriting height from a parent div" starts from a real world use case where a user is wrapping its scrollable pane into a div, which height is inherited by the scrollable pane. If you have the feeling that this is a common design pattern for scrollable views (wrapping a scrollable view into a div, setting the height of the div and setting the ScrollableView? height property to 'inherited'), go along and update the ScrollableView? documentation. If not, I think we can close the issue.

comment:17 Changed 6 years ago by Adrian Vasiliu

  1. As you know (since you have updated this doc in the past), the ref. man. of ScrollableView? already documents its "height" param exactly like ScrollablePane?, including mentioning that it supports the value "inherit". So according to the doc there is no difference with this respect between ScrollablePane? and ScrollableView?.
  2. You can find an example of ScrollableView? with height: inherit in the Accordion view of demos/mobileGallery. I see another example in a big real consulting project.
  3. Since you have worked on this aspect for both classes and you have produced this new piece of doc for ScrollablePane?, it seems to me you are in the best position to know if it applies also to ScrollableView?.
Last edited 6 years ago by Adrian Vasiliu (previous) (diff)

comment:18 Changed 6 years ago by Sebastien Brunot

I don't see the need to document wrapping a ScrollableView? into a div (that is NOT a container widget), and neither do you apparently, so I suggest to close this issue.

comment:19 Changed 6 years ago by Sebastien Brunot

Milestone: tbd1.9.1
Resolution: fixed
Status: assignedclosed

comment:20 Changed 6 years ago by Adrian Vasiliu

neither do you apparently

I don't know why you say that. What I said is rather the contrary: according to our current doc, and according to what we have in mobileGallery and in real projects of users, the use-case of a ScrollableView? with height:inherit wrapped in a div with height in pixels appears to be a supported scenario.

This ticket has been open for ScrollablePane?, so it can of course be closed even if the same issue exists for ScrollableView? and the doc improvement has not been done for it. It's just strange to not apply the doc improvement for both subclasses of dojox/mobile/scrollable if the issue is the same for both.

comment:21 Changed 6 years ago by Sebastien Brunot

Here is the example you referenced in mobile gallery:

<div data-dojo-type="dojox.mobile.Accordion"

data-dojo-props='fixedHeight:true' style="height:400px;"> <div data-dojo-type="dojox.mobile.ScrollableView?"

style="background-color:white" data-dojo-props='label:"ScrollableView", height:"inherit",

icon1:"mblDomButtonBlackRightArrow16", icon2:"mblDomButtonWhiteDownArrow16", propagatable:false, appBars:false'>

=> The scrollable view is not in a "regular" div, it is inside an Accordion widget div. And, as you can see for yourself browsing the live demos, it works. There is no need to document anything for this use case.

comment:22 Changed 6 years ago by Adrian Vasiliu

First of all, as I said, the current doc of ScrollableView? implies that using it in a "regular" div is supported, or at least doesn't imply the contrary. Second, here's the example from the consulting project that I mentioned, which does wrap the ScrollableView? inside a "regular" div: <div class="<a CSS class containing specification of height>">

<div data-dojo-type="<a subclass of ScrollableView?>" ... height="inherit;">

</div>

Last edited 6 years ago by Adrian Vasiliu (previous) (diff)
Note: See TracTickets for help on using tickets.