Skip to content
This repository has been archived by the owner on Jul 30, 2018. It is now read-only.

Implement Geometry Module #8

Closed
4 of 9 tasks
eheasley opened this issue May 22, 2015 · 17 comments
Closed
4 of 9 tasks

Implement Geometry Module #8

eheasley opened this issue May 22, 2015 · 17 comments

Comments

@eheasley
Copy link

Implement the following, along with associated unit tests:

  • geometry.getBorderBox
  • geometry.setBorderBox
  • geometry.getPaddingBox
  • geometry.setPaddingBox
  • geometry.getContentBox
  • geometry.setContentBox
  • geometry.getMarginBox
  • geometry.setMarginBox
  • Support for adjusting based on scroll position in get APIs

The getter implementations would likely begin with getBoundingClientRect (which reports border-box) and build up from there. Dojo 1 does not implement all of these APIs, but it seems more consistent to do so.

dom Proposal Reference

(Note there is an open question in the Rationales section regarding inclusion of a flag controlling behavior related to page scroll. It ought to be feasible to leave that out initially and add common logic to adjust for it in the future.)

@kfranqueiro
Copy link
Member

WIP on https://github.com/kfranqueiro/dom/commits/geometry, currently with basic support + tests for the get APIs.

Dojo 1's dom-geometry.position had an optional flag which would adjust based on the document's scroll position. It did not, however, adjust based on scroll position of any child elements. Need to think through what is necessary/best for this API.

@eheasley eheasley assigned bitpshr and unassigned kfranqueiro May 29, 2015
@kfranqueiro
Copy link
Member

The set functions will need to incorporate some additional concerns:

  • Determine what box-sizing model the element in question is using (via getComputedStyle)
  • Adjust the passed width/height so that they apply correctly based on the current box model and the box that is being set (i.e. the width/height passed is always used to manipulate the element's width/height styles, while leaving any margin/border/padding styles as-is)

For example, if box-sizing is border-box and someone calls setBorderBox, we don't need to adjust. If box-sizing is border-box and someone calls setContentBox, we need to subtract the known values of the borders and padding for the element from the width/height to be applied and also need to subtract top/left border/padding from the top/left to be applied. Also, top/left always affect the placement of the margin-box, so that needs to be taken into account as well.

FWIW, Dojo 1 did not take the box-sizing setting into consideration, and would thus work incorrectly in cases where it has been changed.

@bitpshr bitpshr mentioned this issue Jun 2, 2015
@kfranqueiro
Copy link
Member

Paul has put a bunch of effort into the set* APIs for this as well on his branch (see the referenced PR). However, he brought up an interesting point on one of the remaining edge cases (see last bullet point below), which made us step back for a minute and think about the requirements for these APIs overall.

The problem is, outside of Dijit in Dojo 1.x, we can't really come up with any significant use cases for these APIs (either get or set). We can't recall ever needing to use any of them directly, and would sooner use modern CSS to accomplish much of what Dijit uses setMarginBox to accomplish in 1.x for layout widgets.

Moreover, there are a few loose ends / gray hairs with these APIs:

  • It doesn't make a whole lot of sense to us to be getting/setting top and left via these APIs (incidentally, removing that bit of consideration would also remove the need to concern ourselves with a flag for accounting for page scroll position for get APIs)
  • The set* APIs would be setting inline styles, which goes against best practices, especially given that past use cases involved layout widgets
  • The set APIs typically only ever modify the width and height of elements, never their borders/margins/paddings; however, it would be possible to pass non-negative numbers to these APIs (aside from setContentBox) which would require subtracting borders/margins/paddings in order for the API to "do what it says on the tin", but then which gets subtracted first? (FWIW, Dojo 1's setMarginBox would only ever modify width/height regardless.)

Ultimately, without convincing use cases at this point, it doesn't seem worth opening these cans of worms. I could fathom simplifying the get* APIs to simply returning objects with width and height, but even then I don't have real use cases for it. As such, despite the time we've invested, I'm putting this on hold for now.

I'd welcome comments from anyone who has had real use cases for these APIs in the past, to help figure out whether there's a convincing need for them in Dojo 2.

@rmaccracken
Copy link

The enterprise application I work on uses the "get" APIs in many situations to do various calculations for advanced layouts, dialog sizing, drag-fill editing popups for tables, and splitting up page content for printing. Unless there is a better way to get at this information, I would definitely appreciate the APIs existing in Dojo 2. We do not use the "set" APIs.

Maybe CSS could be better used to solve some of these use cases, but don't think so for all. Migrating to Dojo 2 (if we choose to do so) would be that much easier if they were included.

@dylans
Copy link
Member

dylans commented Jun 15, 2015

@rmaccracken are any of the get API calls you are making things that wouldn't be solved with getBoundingClientRect ( https://developer.mozilla.org/en-US/docs/Web/API/Element/getBoundingClientRect ) ?

@kfranqueiro
Copy link
Member

Adding on to Dylan's question, I'd be curious which get APIs in particular you'd be calling (i.e. which box model you're interested in), and whether you are using them only for width/height as seems to be commonly the case in instances I've looked at.

@speich
Copy link

speich commented Jun 16, 2015

I assume were are not talking about putting dom-geometry.position() on hold, only the set/get API, right?

@kfranqueiro
Copy link
Member

dom-geometry/position primarily consists of a call to element.getBoundingClientRect, optionally subtracting window.pageXOffset and window.pageYOffset if includeScroll is true. Thus, it seemed like including position as-is didn't have much of a point - and the scroll flag raises the question of exactly what it should be accounting for (since deeper ancestors on the page could scroll, too).

Initially we were thinking of including left/top/right/bottom information in each of the get* APIs as a replacement (then getBorderBox would essentially be position, possibly minus the scroll flag), but I've been wondering how widely used the position properties are, relative to the dimension properties (width/height).

@speich
Copy link

speich commented Jun 16, 2015

Ok, thank you for the clarification.

@rmaccracken
Copy link

@kfranqueiro We use getContentBox, getMarginBox, and position (with and without includeScroll). And we do use both the dimension and position properties. As far as the box model, we use the default (content-box). However, in terms of an API, I think I would rather have a function to get the box based on box-sizing rather than what getContentBox does (the comments say it ignores the box model).

@dylans We have always used the geometry APIs rather than getBoundingClientRect, so I don't have any experience with it and can't really say if it can easily replace either getContentBox or getMarginBox. From what is said above, it does look like it could partially replace position. But looking at the getContentBox and getMarginBox source code which does not use getBoundingClientRect, I can't say without testing. From the getBoundingClientRect documentation, it does sounds like it could replace our uses of getContentBox, but I don't think that is the case for getMarginBox.

@kfranqueiro
Copy link
Member

getBoundingClientRect reports border-box, so it wouldn't replace getContentBox or getMarginBox.

What you mentioned about getting the box based on the effective value of box-sizing is an interesting idea. These APIs have been focused on getting exactly the box asked for, but your idea might be more useful for some cases...

@kfranqueiro
Copy link
Member

@rmaccracken: out of curiosity, can you describe the sorts of use cases you have for using the various functions? That's what I've been the most curious about overall. The only cases I've really seen lately are for unit tests.

@rmaccracken
Copy link

@kfranqueiro I mentioned some of the use cases above, but I can go into a bit more detail on our main use case. Our product offers mainly 3 view types (layouts) that are used throughout - grid, gallery (something like google images), and property (shows images/properties belonging to single object). In addition, we have composite views that combine any number of these views together into a larger view. These views all have various options that affect how they are displayed. Some of these options cannot be satisfied by CSS, and so Javascript takes over to arrange the content as desired. Some of those calculations require these geometry functions to get the optimal layout and make sure everything lines up.

In addition, we have the requirement that all of these views need to be printable, so that the content adapts to the chosen paper size/orientation and is split across pages so as to not crop content or span pages in an undesirable way. The code we use to split the content uses the geometry functions as well.

There are various other places as well that use the functions, such as for dialog positioning, drag-fill editing as in Excel, and the list goes on. If our app has at least 10 different use cases for them, then I would think others have them as well.

Our code base has been around for a while, so we may be able to take another look at these use cases and find ways to simplify or remove their use (through CSS or otherwise), but I highly doubt we could get rid of all cases.

@kfranqueiro
Copy link
Member

I'm definitely intrigued by the "Some of these options cannot be satisfied by CSS" statement. I would expect that dialog, tab container, and border container layouts should generally be achievable via CSS at this point using either position-related styles or flex-box. The one potential exception I can fathom would be anything intended to be draggable/resizable, which would still need JS.

The print arguments may be valid, though I wonder whether page-break-after, page-break-before, and page-break-inside could help.

The thing that makes the set functions awkward to me is that if they're expected to set not only width/height but also top/left, they pretty much need to assume that position is already set accordingly anyway, and they would be adding inline styles. Well, that and the third bullet in my earlier comment.

@rmaccracken
Copy link

I'll just say that we currently use those functions, and unless we refactor/redesign all the various places we use them (if even possible), then we will need to recreate them. If they are not in Dojo 2, we will just have to add them to our own codebase. I would of course rather have them in Dojo though. We don't need the "set" functions, so if you are considering dropping these functions just because of those, then I would say just drop the "set" functions and keep the "get" functions.

Anyways, I've added my 2 cents and I'll just wait and see what you decide and go from there.

@kfranqueiro
Copy link
Member

We don't need the "set" functions, so if you are considering dropping these functions just because of those, then I would say just drop the "set" functions and keep the "get" functions.

This is useful feedback, thanks. I think we're still more on the fence about the get functions and far less sold on the set ones if they're rarely used anyway.

Dojo 2 is our first chance in roughly a decade to clean house and not be chained down by backwards compatibility, which is why I'm trying to nail down as much concrete information as possible on use cases that can't already be fulfilled natively. It's easier to add APIs later, whereas dropping things would ostensibly need to wait for another major version bump, hence the attempt to avoid unnecessarily increasing the API surface of 2.0 immediately out of the starting gate.

@kfranqueiro
Copy link
Member

I'd been thinking about this a couple of days ago and figured I would lay out a few options in terms of how far the get APIs in this module should go. (The feedback so far on this ticket and IRC has shown interest primarily in the get APIs, so I'm leaving the set APIs aside.)

The following list goes from least effort to most:

  1. Don't implement the module at all
  2. Implement each of the get*Box APIs to only return dimensions (width/height)
  3. Implement each of the get*Box APIs to return position and dimensions (left/top/width/height/right/bottom, as in my initial branch)
  4. Implement each of the get*Box APIs to return position and dimensions, and include an includeScroll flag
    • The big question here is what that flag should do to be the most useful. In Dojo 1, all it does is subtract document.pageXOffset and document.pageYOffset, but should it account for intermediate ancestors with scrolling too?

If you have opinions on what level of support your application needs, feel free to chime in. Personally I'd prefer to stay away from 4 if possible (if you need the level of support that Dojo 1's dom-geometry would give you with that flag, all you need to do is subtract document.pageXOffset and document.pageYOffset yourself).

@eheasley eheasley modified the milestones: Milestone 1, Milestone 2 Jun 24, 2015
@eheasley eheasley removed this from the Milestone 1 milestone Jun 24, 2015
@kitsonk kitsonk modified the milestones: 2.0.0, Milestone 2 Mar 11, 2016
@kitsonk kitsonk modified the milestones: 2016.08, 2.0.0 Apr 8, 2016
@dylans dylans removed this from the 2016.08 milestone Oct 24, 2016
@dylans dylans added this to the long-grass milestone Jan 12, 2017
@kitsonk kitsonk closed this as completed Sep 4, 2017
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests

7 participants