Prefer 'right' for positioning inside RTL nodes (Fix #1569) #1570

Open
wants to merge 3 commits into
from

Conversation

Projects
None yet
2 participants
Contributor

jbalsas commented Jan 22, 2014

This is a working fix for #1569. (Check this example about the issue)

It switches to position the node using a right-based calculation for nodes found inside RTL blocks, so that the inline position has preference.

I was looking to add some tests as well, but as far as I could see, the ones inside dom-xy-tests.js aren't exactly testing this over existing nodes, but over newly created ones. Is there a place for such tests or should we add them right there with the existing ones?

Contributor

jbalsas commented Jan 22, 2014

This could potentially fix #1539 as well

Contributor

msweeney commented Jan 23, 2014

@jbalsas Thank you for submitting this patch. Unfortunately, I don't think this is the right approach, as the overhead of the ancestor traversal for the direction attribute will bottleneck the most common use-case.

Assuming that the direction of the page is known in advance, a better approach might be to statically configure the offset property.

Something like:

Y.DOM.XY_OFFSET = 'left';

and use that variable in the setXY() call to setStyle():

setStyle(node, Y.DOM.XY_OFFSET, x - currentXY[0] + delta[0] + 'px');

This way developers in an RTL environment can set the value to right, assuring optimal performance for both cases.

Contributor

jbalsas commented Jan 23, 2014

Hi @msweeney, thanks for looking into this!

Yeah, I also wondered about the performance penalty... the problem with setting the variable to left or right is that it won't work for cases were you have different directions inside a page. For instance, in this example, one of the cases will always fail unless we resort to a case-by-case calculation.

Would it be okay if we calculate the direction by default (this way it'll always work properly), and then, if the developer is sure about the direction of the page, he can set Y.DOM.XY_OFFSET and skip it to get some performance improvement?

Another option could be to have Y.DOM.XY_OFFSET = 'left' | 'right' | 'both', where we calculate the direction only in the both case, and then we can default to whatever we think it's the most common case.

What do you think?

Contributor

msweeney commented Jan 24, 2014

Actually, we may be able to skip the ancestor traversal altogether and rely on the computedStyle value. We would need explicit unit tests for each case, but I ran some basic tests, and the correct value is reported from Y.DOM.getComputedStyle() in IE7, latest Chrome, Safari, and Firefox for both the direction style property and the dir attribute on the element or an ancestor.

Assuming this is true, the test could be reduced to just checking for rtl, otherwise default to ltr. This still adds DOM read overheard however, so could negatively impact performance.

Contributor

jbalsas commented Jan 27, 2014

Hey @msweeney, that's good to know... based on getComputedStyle support, I assumed it shouldn't be working on IE8< but I'll check that out.

About the fix, I was trying to come up with a good test suite, and I've found more wrong cases with the current implementation.

Please, check this gist to see what I mean. Out of 12 cases, 5 fail to be positioned properly (as I understand it) with the current setXY implementation. Also, unfortunately, my fix fails to account for two cases and swaps the other three. I'll be looking into making this more generic and adding some more cases to make sure we cover everything.

What do you think? Do you consider those cases as valid errors, or am I missing something?

Contributor

jbalsas commented Jan 31, 2014

@msweeney I've pushed a couple of extra commits, one with the test cases and another with an improved and nicer logic that works for all of them. There are still thinks that could be ironed out. What do you think?

I'll keep working on adding extra cases (or reuse the ones in dom-xy.html) to see if everything works as expected.

Contributor

msweeney commented Jan 31, 2014

@jbalsas This looks better. Since this is a critical path for higher-level features like drag & drop and animation, we should also make sure there is no performance degradation.

Contributor

jbalsas commented Feb 25, 2014

Hey @msweeney, sorry I let this linger for a couple of weeks...

What do you think would be the best approach for testing the performance impact? Do we have any kind of infrastructure already in place? If not, would some test suites in jsperf.com be enough?

Also, I think that if we're worried about performance, setting the Y_DOM.OFFSET_XY to left by default would produce the same execution path we currently have, so we shouldn't suffer any kind of performance degradation, right?

Contributor

msweeney commented Feb 26, 2014

Starting with jsperf is fine. I also anticipate little to no hit for the default path as you mention, but we should validate that.

Contributor

jbalsas commented Feb 26, 2014

Hey, @msweeney, I've created the following jsperf test cases

  • Based on the results on my machine, it looks like we suffer a bigger penalty than expected in IE
  • I can't really explain the discrepancy between the Baseline and the Patched Baseline, which I expected to be closer, and faster than the Patched test.

What do you think? Could you please take a look to see if I'm missing something?

Contributor

jbalsas commented Mar 13, 2014

Hey, @msweeney, did you have the chance to take a look at this?

What else do you think we should do to move this forward?

Contributor

jbalsas commented Jun 6, 2014

Ping @msweeney

Any word on this? We'd like to dig into this so we can avoid patching every new version of yui whenever we update our fork...

Contributor

jbalsas commented Jun 6, 2014

Hey @andrewnicols, thanks for looking into this!

Afaict you're referring to an old piece of code. The logic for that was improved in the last commit https://github.com/jbalsas/yui3/commit/887aa0e8ec6fb6f716969d710398e8f3ae0975cc

I've checked the case you mention, and it's resolving the direction properly.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment