Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Stabilize and enable intersectionObservers #249

Closed
wolfbeast opened this issue Apr 24, 2018 · 14 comments

Comments

Projects
None yet
4 participants
@wolfbeast
Copy link
Member

commented Apr 24, 2018

Painted into a corner, but this should work.

Intersections have been moved to be handled with display lists in FF 18. Mostly because a per-update check of visibility is disastrous for some corner cases (it seems to be the more logical approach to me, but hey...).

As a result off-screen animations will be updated and painted. This can take 100% CPU in some cases (e.g. Facebook loading throbber). So, the display lists need an intersection check anyway in the form of an intersectionObserver.

We have the base code for it, but it's crashy. (damn hash tables again).
So... Fix https://bugzilla.mozilla.org/show_bug.cgi?id=1322717 and we can then likely enable this for UXP.

@wolfbeast

This comment has been minimized.

Copy link
Member Author

commented Jun 27, 2018

Looks like there's a bit more crashiness to be solved.

@wolfbeast wolfbeast self-assigned this Jun 27, 2018

wolfbeast pushed a commit that referenced this issue Jun 27, 2018

wolfbeast
Stabilize and align Intersection Observers
- Fixes several crashes
- Aligns the feature with the W3C WD spec

Tag #249

@wolfbeast wolfbeast closed this in ef29d0a Jun 27, 2018

@wolfbeast

This comment has been minimized.

Copy link
Member Author

commented Dec 21, 2018

This is still disabled after issues with the unstable spec and Chromium-based implementation preferences at the time with breaking changes. We should revisit this, see what the spec has settled on now, and implement that, then flip it back on. If big sites still have issues with it we'll have to either point them to the spec or adjust where needed if we're still non-compliant.

@wolfbeast wolfbeast reopened this Dec 21, 2018

@wolfbeast wolfbeast removed this from the PM 28.0.0 release milestone Dec 21, 2018

wolfbeast pushed a commit that referenced this issue Dec 22, 2018

wolfbeast
Map intersectionRect to the coordinate space of the target document.
Spec says: "Map intersectionRect to the coordinate space of the
viewport of the Document containing the target."
Tag #249

wolfbeast pushed a commit that referenced this issue Dec 22, 2018

wolfbeast
Use content area as the intersection rectangle ...
... for custom root with overflow clip.
Tag #249

@wolfbeast wolfbeast closed this in 4319edc Dec 22, 2018

@Sa-Ja-Di

This comment has been minimized.

Copy link

commented Dec 23, 2018

I would say this one still leads to crashes. Before i have updated to the unstable with this enabled, i never experienced a crash here:

https://www.robtex.com/ip-lookup/5.79.119.119

Getting random crashes when opening/closing such robtex pages with the 64 Bit unstable. Deactivated now

dom.IntersectionObserver.enabled

and the crashes are gone again.

@wolfbeast

This comment has been minimized.

Copy link
Member Author

commented Dec 23, 2018

Yup, it does, and the cause is Mozilla's fucking hashtables again. I'm getting really tired of the crashiness of those things.
At least the crash is easy to track. I'd honestly rather deal with crashes than things "not working as they are supposed to".

@wolfbeast

This comment has been minimized.

Copy link
Member Author

commented Dec 24, 2018

Latest unstable should have this fixed with commit 3cf7e87

@kn-yami

This comment has been minimized.

Copy link
Contributor

commented Dec 24, 2018

Is isIntersecing implemented in UXP?

@JustOff

This comment has been minimized.

Copy link
Contributor

commented Jan 16, 2019

It seem this still breaks sites in the wild, see https://forum.palemoon.org/viewtopic.php?p=159684#p159684.

@JustOff JustOff reopened this Jan 16, 2019

@wolfbeast

This comment has been minimized.

Copy link
Member Author

commented Jan 17, 2019

The issue there is a spec change that added isIntersecting as a property, because apparently it's too difficult to understand for webmasters that 0% intersection equals not intersecting. 😝

wolfbeast added a commit that referenced this issue Jan 17, 2019

Add isIntersecting property to IntersectionObserverEntry.
Per updated spec.

This resolves the issue raised in #249.
@wolfbeast

This comment has been minimized.

Copy link
Member Author

commented Jan 17, 2019

Is isIntersecing implemented in UXP?

I completely didn't see this. Sorry.

I guess the answer is: "it is now!" 😀

wolfbeast added a commit that referenced this issue Jan 17, 2019

Add isIntersecting property to IntersectionObserverEntry. (uplift)
Per updated spec.

This resolves the issue raised in #249.
@JustOff

This comment has been minimized.

Copy link
Contributor

commented Jan 17, 2019

As far as I can see f6ef8d8 resolved the https://forum.palemoon.org/viewtopic.php?p=159684 issue 👍

@wolfbeast

This comment has been minimized.

Copy link
Member Author

commented Jan 18, 2019

I'm going to disable IntersectionObservers in 28.3.1 because it's causing too many crashes and crash-evaluation is currently difficult.
Potentially, if the crashes (seems like a CC problem) can be resolved, they can be re-enabled in 28.4, but it'll need some time to be properly checked on various sites. Keeping it enabled on master so the unstable channel can be a testbed.

@wolfbeast

This comment has been minimized.

Copy link
Member Author

commented Jan 19, 2019

Clearing milestone for this and putting it on hold -- I've been unable to find a solution for #934 and #935 so far and have run out of avenues to explore. I'm probably missing something blatantly simple but can't see it.

Considering this feature is WD status, experimental, and so far only being used for less than user-interest focused purposes (tracking, ad-visibility flags, potential profiling by observing scroll position, etc.), it's not a great loss.

This will be disabled by default on master, also.

@wolfbeast wolfbeast added the On Hold label Jan 19, 2019

@wolfbeast

This comment has been minimized.

Copy link
Member Author

commented Jan 19, 2019

Looks like I've worked around both crashes now, so this can be released again.

@wolfbeast wolfbeast removed the On Hold label Jan 19, 2019

@wolfbeast

This comment has been minimized.

Copy link
Member Author

commented Jan 21, 2019

Both crashes are verified fixed, considering this solved and safe to keep re-enabled

@wolfbeast wolfbeast closed this Jan 21, 2019

wolfbeast added a commit that referenced this issue May 15, 2019

Map IntersectionObserver rect to the correct viewport.
targetFrame is modified during the intersection computation loop, so
it's not the viewport you want if there are scrollframes around.
This bug triggers when IntersectionObservers are used on frames that
wrap.

Follow-up for #249.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.