According to https://www.mediawiki.org/wiki/Compatibility we can:
* Ignore Internet Explorer as well as "Edge legacy" (before it
switched to Chromium) entirely.
* Ignore old Opera (Presto, before it switched to Chromium).
* Ignore Chrome and Chromium-based browsers that are more than
3 years old.
According to https://github.com/othree/jquery.rtl-scroll-type
* Firefox and Safari always followed the web standard.
* The "reverse" type was only ever relevant in IE.
* The "default" type was only relevant in old Chrome and
Chromium-based browsers, before the engine was changed to follow
the web standard.
According to https://chromestatus.com/feature/5759578031521792 this
happened in version 85, June 2020.
The only edge-case I can think of is that we want to support some
niche browser that – for some reason – still uses Chromium 84 or
older. But according to https://www.mediawiki.org/wiki/Compatibility
we are at 88+ already, everywhere.
It appears like we can safely remove two types, which means only
the web standard behavior (a.k.a. "negative") is left.
No pressure merging this. This patch can as well sit here for
another year. ;-)
Bug: T352169
Change-Id: Ifdedfd6d16abc87576df9807a55cd1b8a7d185db
This is closely related to Ied0b974 which fixed a similar, if not
the same rounding issue.
Note the following might be different depending on e.g. the
operating system. My Ubuntu+Chromium shows the following behavior:
* The RTL scroll type is correctly detected as "negative" with all
zoom factors below and up to 100%.
* When the zoom factor is 110%, 125%, or 150% the scrollLeft value
is not 0 but something like 0.909090876 or 0.200000002.
* It's 0 again at 175% and 200%.
* Bad at 250%. Good at 300%. Bad at 400%. And so on. No rhyme or
reason.
The current Firefox version also ends in the "negative" branch, but
doesn't have the same rounding errors. It's always a perfect 0 in
Firefox.
This makes it look like a bug in Chrome's engine. We don't know how
old it is, but based on the information in T352169 it might be a
relatively new bug that didn't exist when this code was originally
written in 2016 (see I7c903c2).
For reference, this is what's supposed to happen here: Browsers with
the scroll type "negative" (which are apparently all current Chrome
and Firefox versions) won't allow scrollLeft to be a positive number
on an RTL page. When you scroll to the left in such browsers the
numbers get negative. The detection code tries to set the number to
+1 anyway. We expect the browser to ignore this invalid call and
still report the previous 0.
This mostly works in Chrome as well. For example, setting scrollLeft
to +100 wont set it to +100 but to … some random number between >=0
and <1, depending on the current zoom factor? o_O?
I suspect we can remove this detection code entirely, or at least
change the default to "negative". But this needs more testing with
more browsers. Let's start with this tiny fix.
Bug: T352169
Change-Id: I22cbb8881578e96165097d4fcc812baadc22d7fa
Most callers use it as if it's a `getElement` call anyway.
There is one .initialize method left in the HelpDialog class. That's
part of the upstream OOUI Window interfaces.
Change-Id: I5727c59ad0ad05d712d51d255906ddc44e898668
This line was added in 2017 via I9adbd47. I think this was a mistake.
What happens is that the grab cursor appears as part of the hover
effects (the yellow/blue "ghost" circles). But these can't be
dragged. Having the cursor there is confusing as it constantly
flickers between the click and drag cursors.
Am I missing something?
Change-Id: I611fee6f7199d68b61b103bcbb2011c88651883b
It looks like there is nothing special about this module. It is
loaded the same time as the other "lazy" module, under the same
conditions.
Change-Id: Iae3e425297ef5ed3f35cb8c8d66b390875158905
In RTL mode the slider is flipped and goes from right to left. The
revisions increase from right to left. The left cursor key needs to
be the one that increases the position.
Note there are three different "RTL scroll types" called "default",
"negative", and "reverse". This is only about CSS positions and
doesn't affect the cursor keys.
Change-Id: Ie747aa2572f2542b6c2c2176f817dd9a42b29f78
Hover and click events to highlight or change revisions need to
know the intended revision target. This did not work for the area
where the pointer containers overlap the underlying revisions.
For that reasons we implemented a calculation to get the revision
using the mouse positions. That implemetation seems to be faulty
at some points.
pointer-events: none allows us to pass though the mouse events so
that we're able to always rely on the revision containers as target.
On the pointers we still want to catch events to allow dragging.
Bug: T352169
Change-Id: Ie53a6ec3b7c458dc2f72e494829dfab80952b86f
When we load new revsions in the background and extend the view,
we still want to keep track of the positioning. This is done with
the `data-pos` attribute. When shifting the numbers due to
re-loading the `data-pos` of each revision is updated to a new
value.
$.data() does not nessecarily load the value from the `data-`
attribute, but seems to have a different representation in parallel
In some situations the results of data() vs attr( 'data-' ) seem
to differ, leading to situations where the pointer moves to the
wrong postion when clicking.
Trusting the attribute call seems to be the most stable approach
here.
Change-Id: I3c877b5a5f21ca36b8857d29ec018b16ff588962
This is split from I4431882 to make that easier to review. This just
moves existing code around. It should not make any difference.
Change-Id: Ic1a63bac4c30656533c2323a5a133aa483a26975
This is split from I4431882 to make that much easier to review.
Additional changes:
* Remove plural "s" from setRevisionPreviewHighlight
* Add "All" to removeAllRevisionPreviewHighlights
* Merge two methods into a single enableRevisionPreviewHighlights
Change-Id: I7088b23a330a46fadfc4ae296cf1d61f0be435f8
This does the same as before, just in one single loop instead of so
many different steps.
The way the loop counts is odd. Why are the numbers returned by
getOldestVisibleRevisionIndex and getNewestVisibleRevisionIndex
off by one? I tried to make this more visible in the code without
touching it.
Change-Id: I5b9bd360bf48e138a7ae9406eab716d41fabd122
The extra "div.…" doesn't make the selectors more specific or
anything. The class name alone is unique. This makes the code
simpler, less brittle, and potentially even faster.
Change-Id: I38f61a793a3f5d441b9bf06f9159433cb7f002ad
This code is updating the position and length of the yellow/blue
horizontal lines in the middle of the slider, left/right to the
yellow/blue circles.
1. setSliderLineCSS is called two times. A lot of the code before
is identical. Move all code that is identical into setSliderLineCSS.
2. The two single-use methods are very short now. Nothing but a
single function call. Merge the two calls into a single method.
Notice that `-0.5 * this.revisionWidth` is the same as
`-this.revisionWidth / 2`. I was able to make one disappear by
slowly transforming the code. It still does the same as before.
Change-Id: I9f9593bf4655fc8fa681fa8190db44dc8d6fc232
Storing a new entry in the browser history is the normal, default
thing to do. The only exception is on initialization, when the slider
is initialized from the browser history.
I hope this makes the code a little easier to read.
Change-Id: I09a3e9b4417ec3d57e86dc947ac0748f30ef0dd5
This click handler is assigned to many different elements. Not all
of them can be used as a source for a data-pos="…" attribute. But a
lot can.
Again, this patch alone will not actually fix T352169. But it will
improve the situation a lot, according to my local tests. Many of
the clicks will start to work fine on RTL wikis because the
problematic pixel-based getRevisionPositionFromLeftOffset
calculations are not used any more.
Steps to reproduce: Go to a RTL page with a very long history, e.g.
https://he.wikipedia.org/wiki/Special:Diff/38031767?uselang=he
Click on one of the gray bars in the right half of the slider. This
will not work, i.e. the slider will not move to this position but
to a totally different one. This is the bug described in T352169.
I noticed that a browser zoom other than 100% can cause many more
problems that are unrelated to this patch. Please test with 100%.
Bug: T352169
Change-Id: Ife49557c891736bc94df6087658f76326791f61b
This updates the code a little bit to be closer to the current
version of https://github.com/othree/jquery.rtl-scroll-type
without actually changing anything (for now).
I tried copy-pasting the current version to see if that helps with
T352169, but it doesn't make a difference.
The only actual change is that the dummy element is removed when
it's not needed any more.
Change-Id: Ibdd064e228fa9464a8652ce9a8a9ac388662f29d
I have seen this getting fed with fractional numbers like
38.9994375. This apparently happens when specific browser zooms
are used, e.g. 110% in Google Chrome. There is a lot of code that
expects this to be an integer number and stops working entirely
when it isn't.
I'm also updating two related comparisons to not be so extremely
specific any more. This probably doesn't make a difference any more
with the fix above, but can't hurt.
This patch doesn't solve T352169, but is one more puzzle piece on
the way to solve it.
Bug: T352169
Change-Id: Ied0b9748beec941e901ca4ecba428c16967ca510
The version numbers mentioned in the comment are from the V8 engine.
The last Google Chrome version to use a 5.x version of the V8 engine
was Google Chrome version 59. That was 8 years ago. See T168299.
Our official support matrix asks for 3 years.
https://www.mediawiki.org/wiki/Compatibility#Browser_support_matrix
Notice how there is no version number in the code despite the comment
explaining that it shouldn't be used in later versions.
According to my local tests this is not a full fix for T352169, but
notably improves the situation. I can still see the bad behavior
described in T352169, but only in a narrow region on the right side
of the slider. Removing the obsolete browser detection is necessary
to unblock further investigation.
Making the dummy text a bit longer also makes an actual difference.
To keep this patch as minimal as possible all I do is to add a single
character.
Bug: T352169
Change-Id: I56f3c1969ce4f164f4319e5038d0f97527e6b1c0
The method is only called from two places, and both do very similar
things before the call. This duplication can be merged.
One notable difference is that .scrollLeft() was possibly called
twice before. The first call was pointless anyway. The argument for
.scrollLeft() is absolute, not relative.
This code cleanup is motivated by T352169, but doesn't solve it.
Bug: T352169
Change-Id: I75e9ffc77ef6331f14e074921c78c28233e60840
