Compensate for body tag with position absolute, relative or static #150

jasco · 2016-08-25T03:52:14Z

In cases where the body tag has a position style of absolute, relative or static adjusts the positioning to compensate for position offsets and borders so powerTip remains properly placed.

Also opts for making all placement based on top rather than bottom attribute. The placement can be adjusted to use bottom if there is some reason to prefer it. The tests have also been adjusted.

This extra flexibility was needed because the control is used in pages where I do not control the html.

Comments or suggestions for improvement welcomed.

stevenbenner · 2016-10-09T19:43:15Z

This looks real good, and would be a huge improvement. I need to do a detailed code review. I do have a concern from the first-pass:

Tips that were using bottom for CSS position now use top.

The reason that they were using bottom was because it is more reliable. If we use bottom for the northern tooltips then if the content in the tooltip changes shape the tooltip will remain in the correct position. (See: Improve resize on tooltip content change - use appropriate corner to pin tooltip, depending on gravity #31 for this original disucssion)

Would it be possible to refactor this to keep using the bottom CSS rule on north-* tooltips?

jasco · 2016-10-17T21:45:24Z

@stevenbenner The positioning has been updated to use bottom for north placements. The branch has also been rebased to current tip of master.

jonwolfe · 2016-11-04T20:27:05Z

@stevenbenner thanks for the great library. We've incorporated PowerTip into a new project, and are experiencing this issue.

As we decide whether to implement a workaround or do a temporary monkeypatch of the current PowerTip release, it would be great to know if you plan on accepting this PR.

Thanks again.

stevenbenner · 2016-12-05T02:11:12Z

@jonwolfe Yes, I do plan on accepting this pull request. However it won't be in the next version (1.3.0). It's so close to release, and this change has more risk than I'm comfortable with for 1.3.0. Hopefully it will get in for version 1.3.1.

jasco · 2016-12-09T17:23:49Z

As an aside, perhaps higher risk commits fit better in a major revision release rather than a minor one?

In any event, it turns out that some webpages position the html tag rather then the body tag. I have a reworked patch that checks both body and html. Would you prefer that I submit a separate pull request or update this one?

stevenbenner · 2016-12-12T04:31:22Z

@jasco How big is the change? If it's a significant rewrite of this patch then go ahead and open a new pull request. Might be educational for me to see the changes. If it's just tweaking a couple lines then go ahead and update this pull request.

jasco · 2016-12-12T16:33:55Z

The technique is the same but the compensation calculation code was significantly restructured. In order to check body and html tags in turn the CSS position check and offset calculation were decomposed into functions. Upon further consideration I think it makes sense to update this patch since the updated implementation really does supersede this one because the checks are ordered. I will keep the commit history so if you can see both versions.

Since I am not sure the issue was clearly articulated before, I'll summarize. The issue is that absolute positioning is relative to the closest, non-static ancestor. Only when all ancestors are static is it relative to the document origin. The compensation adjusts the coordinates relative to document origin to the coordinates relative to the anchoring element.

Edit: fixed misuse of terms

stevenbenner

This looks pretty good. I found one bug that I'd like to get your feedback on, and made a couple other little notes.

One other thing I'd like to change is the extra test pages. These are probably not necessary. I'd rather modify the current edge-cases.html file to include a css toggler. That way I can review the whole suite of extra test cases with the new position handling.

I can do this later if you don't want to go through the trouble. The idea would be to add a select box, radio, or button that would then set or remove the CSS position on the body and/or html tags.

stevenbenner · 2017-01-29T19:29:56Z

src/utility.js

+ * @private
+ * @param {number} windowWidth Window width in pixels.
+ * @param {number} windowHeight Window height in pixels.
+ * @return {Offsets} The top, left, right, bottom offset in pixels


"Offsets" isn't a type in this context. This should be "Object". Example:

@return {Object} An object with top, left, right, bottom offset values

stevenbenner · 2017-01-29T19:33:36Z

src/utility.js

+	// position value other than static. Whether the element is positioned is
+	// relevant because absolutely positioned elements are positioned relative
+	// to the first positioned ancestor rather than relative to the doc origin.
+	var isPositioned = function(el) {


I'd like to see this function pulled out of the computePositionCompensation() function. This has a couple benefits:

(Big one) It can be unit tested on it's own.

It makes sure that the function is only generated once at run time.

Just move it to be another utility function in this file.

stevenbenner · 2017-01-29T19:34:20Z

src/utility.js

+		return el.css('position') !== 'static';
+	};
+
+	var getElementOffsets = function(el) {


x2 - Same thing as above. I'd like this function to be moved out of computePositionCompensation() and be a global utility function.

stevenbenner · 2017-01-29T19:50:45Z

src/utility.js

+		// right offset = right margin + body right position
+		offsets.right = (elWidthWithMargin - el.outerWidth() - (offsets.left - elPositionPx.left)) + (windowWidth - elWidthWithMargin - elPositionPx.left);
+		// bottom offset = bottom margin + body bottom position
+		offsets.bottom = (elHeightWithMargin - el.outerHeight() - offsets.top) + (windowHeight - elHeightWithMargin - elPositionPx.top);


Are the elPositionPx values actually needed? They seems to be causing a bug (I'll detail in PR comments).

Looks like I omitted an elPositionPx.top in the second part of the computation but now I see the terms will cancel. Thanks for catching.

stevenbenner · 2017-01-29T20:03:11Z

I've had a chance to test this out and do a code review, and I did find one bug.

Normal

bodyoffset-rel

Env: Firefox 51.0.1

Notice that there is a gap between the tooltip arrow and the element the tooltip opened for on the relative positioned test. I think this is because you're adding the $.postion() value to the returned offsets. I don't see why that is needed, is there a reason you're using elPositionPx?

jasco · 2017-01-31T00:56:13Z

Good catch, thank you. Not sure why I did not notice the offset. It seems that when calculated as I intended that the el*WithMargin and the elPositionPx terms all cancel out. The simplified calculation seems to fix the anomaly.

jasco · 2017-01-31T12:38:04Z

I added the position settings to edge-cases.

It works fine for body but not for the html tag. The jquery offset returns left: 0, top: 0 even when the html tag is positioned. I'm not sure the most robust way to find the position yet. getBoundingClientRect has some potential but I do not yet have the right computation for right and bottom offsets.

I'll push the tests. Maybe you have a suggestion?

stevenbenner

This latest version has fixed the bottom placement bug in my testing. Though I did discover one other bug.

Thanks for adding the positioning tool to tests-edge.html. That is really convenient for my testing.

Since all of the testing can now be done on one page, these files are no longer needed:

test/htmloffset-rel.html
test/bodyoffset-rel.html
test/bodyoffset-abs.html
test/tests-bodyoffset.js

stevenbenner · 2017-02-05T20:19:43Z

src/utility.js

+	// adjusting viewport even though it might be negative because coords
+	// comparing with are relative to compensated position
+	var viewportTop = session.scrollTop - session.positionCompensation.top,
+		viewportLeft =	session.scrollLeft - session.positionCompensation.left,


These seem to be causing the collision detection to break when the position is relative. Will explain the bug in PR comments.

Are you sure this is the correct place to apply the adjustment? These variables (viewportTop, viewportLeft) are the viewport position relative to the page.

(Source: http://www.quirksmode.org/mobile/viewports.html)

Since the positionCompensation adjustments are based on the CSS, I think the adjustments need to be applied in the maths below, which compare the CSSCoordinates values to the viewport position to determine if there is a collision.

stevenbenner · 2017-02-05T20:26:13Z

test/edge-cases.html

 			$('#theme-switcher').on('change', setTheme);
+			$('#position-switcher').on('change', setCssPosition);


Could you also call this function on load (like setTheme() on line 75)? It's nice to be able to reload the page and not have to scroll to the top to reset the position.

Just add setCssPosition() somewhere.

I would think it will need more than the function call. The dropdown value would presumably reset on reload unless the value is saved somewhere. I could use a url parameter to preserve the position setting so it can be restored on reload. Would that be helpful?

stevenbenner · 2017-02-05T20:43:21Z

Thanks for the updates! The latest version fixed that bug, though I did discover another bug in testing.

Smart placement broken in relative positioning

It looks like the smart placement feature isn't working properly for tooltips that use bottom or right css positioning when the page is using relative positioning.

Repro steps:

Open tests-edge.html
Set the position to "BODY relative"
Scroll down to the "Huge Text with Smart Placement" test
Mouse over the "North" button

You'll see that even if there is plenty of room the tooltip will not show in the north position.

I believe this is because the collision detection from the getViewportCollisions() function is broken. This happens when the positionCompensation.top value is large. After reviewing the code I think that the positionCompensation adjustments are being applied in the wrong place (see my comment in the code review).

Side thought

After reviewing the code I've started to think that maybe this new logic belongs in the CSSCoordinates() object. It might be convenient to change the top, left, right, and bottom properties to be methods/getters that automatically apply the positionCompensation adjustments when something tries to read the property. That way you wouldn't have to apply the compensation to every place in code that touches the CSS positioning values.

This is just a thought I had, not a change request. So feel free to ignore.

jasco · 2017-02-14T02:39:00Z

(Rebased to current master. It appeared there was no conflict between this patch and the diff to master.)

Thanks for your review and feedback on this issue. I be out the rest of the week but I'll look at your comments when I get back.

Also opts for making all placement based on top rather than bottom.

As requested in stevenbenner#151 referencing issue stevenbenner#31 for rationale

Also simplified both bottom and right offset calculations by removing canceled terms.

Measurement compensation moved to CSSCoordinates. Calculations redesigned based on study of measurement origin changes based on CSS position, margin and border widths.

jasco · 2017-06-16T10:42:48Z

The compensation calculations and collision detection have been completely revamped. I also took your suggestion to move the compensation computation into CSSCoordinates. The collision detection has been revised to use the same origin for all values include bottom and right. Also, the calculations now accounts for non-zero border widths on the non-static ancestor.

Also, the branch has been rebased to the head of master again.

I hope/think it is right now. I finally feel like I completely understand how measurements work in all of the cases I considered. It has been a rather painful process but I learned something along the way. I would note that, while I don't know of any issues, I have not reviewed for browser compatibility or behavior in legacy doctypes.

Thanks for your feedback so far.

jasco force-pushed the fixup/compensate-for-body-absolute-fixed-relative branch from 0413910 to 2a3f2e7 Compare August 25, 2016 04:27

jasco force-pushed the fixup/compensate-for-body-absolute-fixed-relative branch from 928ff64 to 4a1abf3 Compare October 17, 2016 21:33

stevenbenner added this to the 1.3.1 milestone Dec 5, 2016

jasco force-pushed the fixup/compensate-for-body-absolute-fixed-relative branch 2 times, most recently from d70e601 to d5df67f Compare December 12, 2016 17:09

stevenbenner requested changes Jan 29, 2017

View reviewed changes

jasco force-pushed the fixup/compensate-for-body-absolute-fixed-relative branch from 2a2b5e8 to b6fe987 Compare January 31, 2017 01:02

stevenbenner requested changes Feb 5, 2017

View reviewed changes

jasco force-pushed the fixup/compensate-for-body-absolute-fixed-relative branch from 2a5e424 to 6e920f7 Compare February 14, 2017 02:13

Joel Steres added 7 commits June 12, 2017 12:01

Compensate for body tag with position absolute, relative or static

9f9b2fb

Also opts for making all placement based on top rather than bottom.

Position north side placements using bottom css

66baec7

As requested in stevenbenner#151 referencing issue stevenbenner#31 for rationale

Compensate for positioned HTML element

5bf6a63

Refactor inlined functions to top level per feedback. Fix bottom calc.

2d41b1e

Also simplified both bottom and right offset calculations by removing canceled terms.

Add position test cases to edge-cases

700edb7

Remove obsolete test files. Tests integrated in edge-cases.html.

fcedf69

Reworked positioning and collision computations.

b9713e4

Measurement compensation moved to CSSCoordinates. Calculations redesigned based on study of measurement origin changes based on CSS position, margin and border widths.

jasco force-pushed the fixup/compensate-for-body-absolute-fixed-relative branch from 6e920f7 to b9713e4 Compare June 16, 2017 10:19

Joel Steres added 2 commits January 10, 2018 05:01

Add docstring to compensation function.

ac7faa4

Add docstring returns description to positionedParentViewportHomeOffset

9ff077b

stevenbenner removed this from the 1.3.1 milestone Mar 12, 2018

LufoX11 mentioned this pull request Jan 20, 2022

North positioning issues #181

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Compensate for body tag with position absolute, relative or static #150

Compensate for body tag with position absolute, relative or static #150

jasco commented Aug 25, 2016

stevenbenner commented Oct 9, 2016

jasco commented Oct 17, 2016

jonwolfe commented Nov 4, 2016

stevenbenner commented Dec 5, 2016

jasco commented Dec 9, 2016

stevenbenner commented Dec 12, 2016

jasco commented Dec 12, 2016 •

edited

Loading

stevenbenner left a comment

stevenbenner Jan 29, 2017

stevenbenner Jan 29, 2017

stevenbenner Jan 29, 2017

stevenbenner Jan 29, 2017

jasco Jan 31, 2017 •

edited

Loading

stevenbenner commented Jan 29, 2017

jasco commented Jan 31, 2017

jasco commented Jan 31, 2017

stevenbenner left a comment

stevenbenner Feb 5, 2017

stevenbenner Feb 5, 2017

jasco Jun 12, 2017

stevenbenner commented Feb 5, 2017

jasco commented Feb 14, 2017

jasco commented Jun 16, 2017 •

edited

Loading

		$('#theme-switcher').on('change', setTheme);
		$('#position-switcher').on('change', setCssPosition);

Compensate for body tag with position absolute, relative or static #150

Are you sure you want to change the base?

Compensate for body tag with position absolute, relative or static #150

Conversation

jasco commented Aug 25, 2016

stevenbenner commented Oct 9, 2016

jasco commented Oct 17, 2016

jonwolfe commented Nov 4, 2016

stevenbenner commented Dec 5, 2016

jasco commented Dec 9, 2016

stevenbenner commented Dec 12, 2016

jasco commented Dec 12, 2016 • edited Loading

stevenbenner left a comment

Choose a reason for hiding this comment

stevenbenner Jan 29, 2017

Choose a reason for hiding this comment

stevenbenner Jan 29, 2017

Choose a reason for hiding this comment

stevenbenner Jan 29, 2017

Choose a reason for hiding this comment

stevenbenner Jan 29, 2017

Choose a reason for hiding this comment

jasco Jan 31, 2017 • edited Loading

Choose a reason for hiding this comment

stevenbenner commented Jan 29, 2017

Normal

bodyoffset-rel

jasco commented Jan 31, 2017

jasco commented Jan 31, 2017

stevenbenner left a comment

Choose a reason for hiding this comment

stevenbenner Feb 5, 2017

Choose a reason for hiding this comment

stevenbenner Feb 5, 2017

Choose a reason for hiding this comment

jasco Jun 12, 2017

Choose a reason for hiding this comment

stevenbenner commented Feb 5, 2017

Smart placement broken in relative positioning

Side thought

jasco commented Feb 14, 2017

jasco commented Jun 16, 2017 • edited Loading

jasco commented Dec 12, 2016 •

edited

Loading

jasco Jan 31, 2017 •

edited

Loading

jasco commented Jun 16, 2017 •

edited

Loading