AprilSylph
Tweaks: "Move post permalinks to the attributed blogname"
- blocked by #2008
- see #2005
- see #2006
Problem no.1: People do not like this A/B test, since it does genuinely remove a feature of Tumblr—to be able to select text in a post without visiting its permalink.
Problem no.2: Where else would the post permalink go? Back in the post/trail item headers? We already have a Tweaks option to essentially revert that.
Solution as I see it: change the "Restore links to individual posts in the post header" Tweaks option to also remove the whole-post permalink. This way, nobody is given an option to completely remove permalinks from the post, but the permalink is not attached to any blanket area of the post—the polar opposite of this A/B test.
I think this is worth working on before the feature-in-testing's full launch. On accounts without this feature, this change should do nothing except rename the Tweaks option.
I think I'm missing something. Currently, without the A/B test, clicking the background of a post or reblog header opens the post permalink.
We currently have a tweak which makes clicking the username in the post header (though not the reblog header) also open the post permalink (rather than the linked blog).
With the A/B test, nothing about the header is changed; there is additionally the ability to click the post or reblog body to do the same thing that clicking the post or reblog header would do.
Are these the same assumptions you have about the situation?
Currently, without the A/B test, clicking the background of a post or reblog header opens the post permalink.
Correct.
We currently have a tweak which makes clicking the username in the post header (though not the reblog header) also open the post permalink (rather than the linked blog).
Okay, maybe strike out "On accounts without this feature, this change should do nothing". My off-the-cuff assumption was that it affected trail item headers as well, because that is something I would assume it would do. I am now proposing that we correct my assumption by changing this fact.
With the A/B test, nothing about the header is changed; there is additionally the ability to click the post or reblog body to do the same thing that clicking the post or reblog header would do.
...so, if the whole-post permalinks were removed from the DOM, the post header permalinks would remain? If so, that doesn't seem like it would remain that way after a full launch. (Or, who knows, maybe it would—can't imagine there's much time allotted for pure cleanup at Tumblr these days.)
I'm still missing something!
- Currently, with or without the A/B test, with or without our tweak, clicking the header background opens the post permalink. If we did a perfect revert of the A/B test, we would be back where we currently are: clicking the header background would still open the post permalink.
- We have a tweak that affects what happens if you click the username in the header.
Oh, I think I see the assumption you're making that I'm not: you're asserting that the feature proposed in this issue would, by necessity, remove the click behavior on the header, whether that's because the current A/B test is written that way (as it happens, it's not) or because the launched version could result in this being the case. If this were the case, implementing it would cause the user not to be able to get to the post permalink unless they also had our current tweak enabled.
This assumes that we would not re-implement, ourselves, the header click behavior, whether because that could be difficult or because we don't think it's how the site ought to work.
or because the launched version could result in this being the case. If this were the case, implementing it would cause the user not to be able to get to the post permalink unless they also had our current tweak enabled.
This assumes that we would not re-implement, ourselves, the header click behavior, whether because that could be difficult or because we don't think it's how the site ought to work.
Yes, that's exactly it, sorry that I failed to explain it properly!
No worries! I think I was simply biased towards my reading because I've already spent time investigating this, so my priors were that (as is currently the case in the tumblr frontend) the header click code was entirely separate from the body click code, and so I didn't think to connect them.
On to practical matters: renaming and adding to an existing feature toggle:
- means one cannot be selected without the other, which makes sense only if "person who actually wants to be able to click post/reblog bodies to open the post" is not a sizable or important demographic.
- means people who already had the toggle enabled will suddenly receive this change, which a) makes sense if we think most of them want it—same point as the prior bullet, essentially—and costs some amount of confusion in which people think the A/B is over and/or the feature rollout was reverted
- means we don't, logically, re-implement the header click if it's removed (or the feature toggle name would be incorrect)—this would mean either intentionally removing it as part of the feature, or not doing so but potentially having it vanish in the future if tumblr does cleanup on it, which would be kind of odd, but minor.
My take, at the moment: "shrug emoji, I'll defer to you on it."
- "attributed blogname" doesn't really sound shippable to me. "to the blog link in the post/trail header" is kind of wordy and I don't have any better ideas, though.
Note: I would love to work on this, but I Don't Know How To Do It. (More accurately, I Only Know One Way To Do It And That Way Is Bad—specifically, we could try to convince Tumblr's code that everything is inside an audio post. If you have any ideas about this, April, I'm all ears. Surely there must be something clever we can do to intercept the synthetic events only in the correct case... right?)
Surely there must be something clever we can do to intercept the synthetic events only in the correct case... right?
By "synthetic events", we are talking about React's events, correct? If so, we already are intercepting those in this very tweak:
https://github.com/AprilSylph/XKit-Rewritten/blob/132104b6c208b0688cf08564bba83bff5cc42aa7/src/features/tweaks/restore_attribution_links.js#L12-L21
Because synthetic events are necessarily powered by actual events, if we stop the click event from propagating down to the React root, React will never see them. We also rely on this for the Create button tweak.
The only potential downfall of the above is if the React listener is attached to the capturing phase... if that's the case, we would need to add our own propagation-stopping capturing event listener to the document element, and selectively stop propagation based on the event target.
The problem is that if we intercept synthetic events at the post body, we also intercept events that result from clicking children elements. I was hoping we could attach a handler that would intercept only the correct click events purely by virtue of the way it's attached, but I don't see an obvious way to do this.
(Looking at it now I don't see why I was worried about that—is event.target === event.currentTarget not good enough? Maybe I tried that already and it didn't work, but I don't recall doing so. Possibly ignore all of this.)
(Looking at it now I don't see why I was worried about that—is
event.target === event.currentTargetnot good enough? Maybe I tried that already and it didn't work, but I don't recall doing so. Possibly ignore all of this.)
Okay, never mind, that was totally irrelevant and doesn't work.
Right, let me state this more simply: if this were a real event handler on the post body, if we intercepted the event between the post body child element and the post body, any handlers on post body children would run; any handlers on the post body would not.
Because react uses event delegation, events always go through all of the elements; placing an intercepting event handler anywhere in the DOM hierarchy can't cause a difference in which handlers get fired.
Right, so we essentially have to do our own event delegation before React gets the chance to. If we attach our own event listener to the documentElement in the capturing phase, anything we stop propagation of there should never reach the div#root element, which seems to be where React is listening for events on.
If React is also watching for the event in question on the documentElement in the capturing phase, we might be screwed.
It's not. What you said works; we don't even have to put the listener there or use capturing (the actual element works fine, as then ours will occur before react's in the normal event bubbling phase).
This means, though, that we must make a determination about whether to stop the event's propagation based on the target of the event, because we're intercepting both click events that we do want react to process (like the user clicking on... an image in a post!) and those we don't want react to process (clicking on the body) in a way that matches which react handler would be active inside react's synthetic event system, which we can't touch. It's not obvious to me what the correct logic would be.
(edit: Unimportant, but I just realized that if Tumblr didn't use synthetic events, to prevent my lamenting—say they switched to preact/compat for some reason—we wouldn't have a clean way to intercept clicks on the actual post element, because handlers run in attachment order, so we couldn't stopImmediatePropagation them successfully in the bubbling phase. We'd need two handlers, one without capture and one with capture that checked event.target === event.currentTarget. Well, anyway.)
we're intercepting both click events that we do want react to process (like the user clicking on... an image in a post!) and those we don't want react to process (clicking on the body) in a way that matches which react handler would be active inside react's synthetic event system, which we can't touch. It's not obvious to me what the correct logic would be.
If event.target === event.currentTarget doesn't work (why doesn't it? I would love to know...) then event.target.matches(/* whatever selector we use to attach the listener in the first place */ is, I think, an acceptable alternative. We only want to stop propagation of click events passing through the anchor elements, so we can just check if the event.target is one of said anchor elements... right?
Only other pitfall I can think of to avoid is using an async function as the listener, I would not trust that to work properly even if it appears to.
(I'm assuming the anchor element is one of those position: absolute "cover" anchors, since if they were not that, posts couldn't have other clickable elements in them, because nesting clickable elements is illegal in HTML.)
Ah, I didn't realize you hadn't looked into this yet.
There is no anchor element. (We could just hide it if there was.) Posts are clickable because there's a (React, synthetic) click event handler on the post/trail item div. Clicks are occurring on various post content elements contained within it (i.e. why event.target === event.currentTarget isn't useful) and are bubbling up. The click handler has a bunch of logic in it (that I guess we have to duplicate and invert) that says, "hey, was this click supposed to do something else? No? Okay, open the permalink in blog view."
"Pretending the whole post is an audio player" works because, after a bug report I forwarded through the support form, Staff added a data-audio-player attribute to the audio player and added it to the list of conditions that mean a click shouldn't count. I don't love the non-semantic-ness and thus potential for breakage of this solution. (Aside: searching "data-audio-player" in the firefox debugger tab is a quick way to get to the handler.)
Ah. That sucks.
...knowing this, I think pretending the entire post is an audio player is fine, actually.
Honestly, it would be nice if redpop just happened to have a semantic attribute for it, a la data-skip-glass-focus-trap. If you were still Staff I'd be like 🥺👉👈 (even though I don't love relying on that kind of thing either unless I could also insert a comment into the redpop source like "heyyy this is used by third party extensions, keep that in mind if refactoring it kthx")
"heyyy this is used by third party extensions, keep that in mind if refactoring it kthx"
The good news here is that we can pretty much just rely on nothing ever getting refactored
I think pretending the entire post is an audio player is fine, actually.
On the one hand, I mean, great, that makes the non-CSS portion of this like four lines of code including cleanup, probably!
On the other hand, like, ughhhhhhhh why couldn't this be one of those times where you just go like "no marcus you're being stupid we simply [obvious solution that does not suck and makes actual sense] :P
Okay, it's like 9 lines.
import { keyToCss } from '../../utils/css_map.js';
import { buildStyle } from '../../utils/interface.js';
import { pageModifications } from '../../utils/mutations.js';
// this takes advantage of tumblr's special-case code for audio players
const preventPostClickAttribute = 'data-audio-player';
const value = 'xkit-no-post-body-permalinks';
const postBodyPermalinkSelector = `:is(
${keyToCss('postContent')}${keyToCss('hasPermalink')},
${keyToCss('reblog')}${keyToCss('withTrailItemPermalink')},
${keyToCss('footerWrapper')}${keyToCss('isReblogWithAddedContent')}
)`;
const hasHoverColorSelector = keyToCss('heightRestrictorExpandButtonWrapper', 'contentWarningCover', 'expandTagsButtonWrapper');
export const styleElement = buildStyle(`
/**
* this doesn't have :hover because, on reblogs with contributed content not viewed alone,
* Tumblr syncs the last reblog and footer's hover state using :has().
*/
${postBodyPermalinkSelector} {
background-color: unset !important;
cursor: unset !important;
}
${postBodyPermalinkSelector} ${hasHoverColorSelector} {
background-color: unset !important;
}
/* copied from Tumblr's styles when only headers were clickable */
${keyToCss('postHeader', 'trailHeader')}:hover a${keyToCss('permalink')} {
background-color: var(--content-tint);
}
`);
const processPermalinkElements = elements =>
elements.forEach(element => element.setAttribute(preventPostClickAttribute, value));
export const main = async function () {
pageModifications.register(postBodyPermalinkSelector, processPermalinkElements);
};
export const clean = async function () {
pageModifications.unregister(processPermalinkElements);
$(`[${preventPostClickAttribute}="${value}"]`).removeAttr(preventPostClickAttribute);
};
edit: Note that this CSS breaks Themed Posts, but so does the a/b itself (tracked as #2001).