AprilSylph / XKit-Rewritten

🧰 The enhancement suite for Tumblr's new web interface
GNU General Public License v3.0
274 stars 44 forks source link

Breakage in V2 timelines with endless scrolling off #1488

Closed marcustyphoon closed 5 days ago

marcustyphoon commented 3 weeks ago

Platform

MacOS 14.4.1 arm64

Browser

Chrome 125.0.6422.77

Addon version

0.23.6

Details

Sorry for the overly-technical title; this problem includes some upcoming Tumblr code tweaks and it's too complicated to specify which of its problems occur now and which will occur soon.

In V2 timelines with data-timeline-id, specifically with endless scrolling disabled, each listTimelineObject element is surrounded in a classless <div> element (as opposed to one with data-cell-id when endless scrolling is enabled, or in a V1 timeline regardless of whether endless scrolling is enabled).

This causes our getTimelineItemWrapper utility to fall back to selecting the listTimelineObject element itself, which is fine in a lot of cases, but because it is no longer selecting the outermost element of a post, this breaks, at least:

https://github.com/AprilSylph/XKit-Rewritten/blob/a92be15496450118418166676fb3c978868cb20f/src/scripts/show_originals.css#L1

Presumably, the correct way to do this is to update getTimelineItemWrapper to handle this case correctly. This isn't exactly easy, though, when both the div we want to target and the parent div that we do not want to target have no HTML attributes at all.

Ideas?

AprilSylph commented 3 weeks ago

The structures of elements are consistent within v1 and consistent within v2, right? The only problem is that they're inconsistent with each other? If so, then I think we just split the logic by detecting the Timeline version, which should be pretty easy: v1 always has data-timeline, v2 always has data-timeline-id.

AprilSylph commented 3 weeks ago

(Note: I am not counting attributes when I say "structure", I understand that that's inconsistent within v2; but in v2 there's always an item wrapper div, and we always want to target that, right?)

marcustyphoon commented 3 weeks ago

I don't recall whether there's a situation in which the structure is ever not that, actually. Regardless, so long as if there is a parent data-timeline-id, return closest('div') works, who cares, and I think that's true, yes.

Edit: Or, rather, uh, return parentElement, I guess. Closest including self is always a bit surprising.

AprilSylph commented 2 weeks ago

Closest including self is always a bit surprising.

.closest('div:not(:scope)') :P

marcustyphoon commented 1 week ago

Regardless, so long as if there is a parent data-timeline-id, return closest('div') works, who cares, and I think that's true, yes.

Edit: Or, rather, uh, return parentElement, I guess. Closest including self is always a bit surprising.

Nope: neither of these works if you're calling getTimelineItemWrapper on anything other than a post element, which we do in carousel hiding/carousel modifying code.