Moodle
  1. Moodle
  2. MDL-37664

Drag & drop of blocks doesn't work in right-to-left mode

    Details

    • Rank:
      47369

      Description

      If you're using a right-to-left language and you drag & drop a block from left sidebar to right sidebar (or vice versa) and then refresh the page, you'll see that the block has not moved to the other sidebar.

      in fact, if you move a block from left sidebar to right sidebar and then from right sidebar to left sidebar again and then refresh the page, you'll see that the block has moved from left sidebar to right sidebar.

      This bug is introduced in 2.4 and has something to do with all "right_to_left()" calls in [theme]/layout/general.php

        Activity

        Hide
        Andrew Nicols added a comment -

        Fred,

        Was this the call you introduced in 2.4?

        Show
        Andrew Nicols added a comment - Fred, Was this the call you introduced in 2.4?
        Hide
        Frédéric Massart added a comment -

        Hi Andrew, no it's not. I've had a quick look, and it just seems that yui/blocks/blocks.js@get_block_region() does not make any distinction between LTR or RTL. As the right content column is always called region-post, the block will never move from there. Feel free to assign me as peer reviewer if you're going to fix this.

        Cheers,
        Fred

        Show
        Frédéric Massart added a comment - Hi Andrew, no it's not. I've had a quick look, and it just seems that yui/blocks/blocks.js@get_block_region() does not make any distinction between LTR or RTL. As the right content column is always called region-post, the block will never move from there. Feel free to assign me as peer reviewer if you're going to fix this. Cheers, Fred
        Hide
        Shamim Rezaie added a comment - - edited

        You're right Fred.
        This can be fixed by changing get_block_region's implementation to this:

                get_block_region : function(node) {
                    var region = node.ancestor('div.'+CSS.BLOCKREGION).get('id').replace(/region-/i, '');
                    if (Y.Array.indexOf(this.get('regions'), region) === -1) {
                        // Must be standard side-X
                        var documentbody = Y.one('body');
                        if (documentbody.hasClass('dir-rtl')) {
                            if (region == 'post') {
                                region = 'pre';
                            } else if (region == 'pre') {
                                region = 'post';
                            }
                        }
                        return 'side-' + region;
                    }
                    // Perhaps custom region
                    return region;
                }
        

        but in my opinion it's just a quick and dirty fix.

        Dude... I'm wondering why all those if(!right_to_left()) s have been added to [theme]/layout/general.php!
        RTL is something that should be handled by CSS, not the code. Handling RTL in the code just leads to code spaghettification.

        Show
        Shamim Rezaie added a comment - - edited You're right Fred. This can be fixed by changing get_block_region's implementation to this: get_block_region : function(node) { var region = node.ancestor('div.'+CSS.BLOCKREGION).get('id').replace(/region-/i, ''); if (Y.Array.indexOf( this .get('regions'), region) === -1) { // Must be standard side-X var documentbody = Y.one('body'); if (documentbody.hasClass('dir-rtl')) { if (region == 'post') { region = 'pre'; } else if (region == 'pre') { region = 'post'; } } return 'side-' + region; } // Perhaps custom region return region; } but in my opinion it's just a quick and dirty fix. Dude... I'm wondering why all those if(!right_to_left()) s have been added to [theme] /layout/general.php! RTL is something that should be handled by CSS, not the code. Handling RTL in the code just leads to code spaghettification.
        Hide
        Andrew Nicols added a comment -

        Adding Ruslan here. Block drag and drop is his baby.

        Show
        Andrew Nicols added a comment - Adding Ruslan here. Block drag and drop is his baby.
        Show
        Ruslan Kabalin added a comment - Two commits here: https://git.luns.net.uk/moodle.git/commitdiff/1797113996c5768bc93abb377b9a33da6c9516d8 https://git.luns.net.uk/moodle.git/commitdiff/c446f2dfa00ddebe3b0bd37eae20707a1a7e7b28
        Hide
        Ruslan Kabalin added a comment -

        Thanks Shamim for the patch, I have converted it to commit! No idea about right_to_left() function, looks like a historic stuff (~2007) and there are 72 occurrences of it in the core. May be we need a ticket for RTL re-factoring if handling it via css only will be possible.

        Show
        Ruslan Kabalin added a comment - Thanks Shamim for the patch, I have converted it to commit! No idea about right_to_left() function, looks like a historic stuff (~2007) and there are 72 occurrences of it in the core. May be we need a ticket for RTL re-factoring if handling it via css only will be possible.
        Hide
        Ruslan Kabalin added a comment -

        Can be cleanly cherry-picked to 2.4.1

        Show
        Ruslan Kabalin added a comment - Can be cleanly cherry-picked to 2.4.1
        Hide
        Frédéric Massart added a comment -

        Hi guys,

        thanks for your work on this. Just a few minor comments:

        Cheers,
        Fred

        Show
        Frédéric Massart added a comment - Hi guys, thanks for your work on this. Just a few minor comments: The commit message should display the component (block probably here) http://docs.moodle.org/dev/Commit_cheat_sheet#Provide_clear_commit_messages Wouldn't that need to be patched for 2.3 too? If so, could you provide a patch? Just FYI, in 2.4 onwards there is a JS function right_to_left() as well. Cheers, Fred
        Hide
        Ruslan Kabalin added a comment -

        Thanks Frederic, I have added branches for stable releases. Thanks for pointing at existing JS function.

        Show
        Ruslan Kabalin added a comment - Thanks Frederic, I have added branches for stable releases. Thanks for pointing at existing JS function.
        Hide
        Frédéric Massart added a comment -

        Thanks Ruslan, pushing for integration now. My personal style usually goes for if/else instead of if/elseif, but that's just me and the region should never be something else than post or pre.

        Cheers!
        Fred

        Show
        Frédéric Massart added a comment - Thanks Ruslan, pushing for integration now. My personal style usually goes for if/else instead of if/elseif, but that's just me and the region should never be something else than post or pre. Cheers! Fred
        Hide
        Eloy Lafuente (stronk7) added a comment -

        The main moodle.git repository has just been updated with latest weekly modifications. You may wish to rebase your PULL branches to simplify history and avoid any possible merge conflicts. This would also make integrator's life easier next week.

        TIA and ciao

        Show
        Eloy Lafuente (stronk7) added a comment - The main moodle.git repository has just been updated with latest weekly modifications. You may wish to rebase your PULL branches to simplify history and avoid any possible merge conflicts. This would also make integrator's life easier next week. TIA and ciao
        Hide
        Eloy Lafuente (stronk7) added a comment -

        Integrated (23, 24 & master), thanks!

        PS: Isn't it a bit hacky that way? I'd expect everything to continue working without any code like that. Anyway...

        Show
        Eloy Lafuente (stronk7) added a comment - Integrated (23, 24 & master), thanks! PS: Isn't it a bit hacky that way? I'd expect everything to continue working without any code like that. Anyway...
        Hide
        Rajesh Taneja added a comment -

        Sorry Ruslan,

        It works fine in master, but in 23 it is not working for both RTL and LTR.
        Please see attached video.

        Also, in RTL on course settings page (There are no blocks on left side.), but user can D-n-D blocks on left side.
        In Stable there was no error, but refresh page put moved block on right bottom.
        In Integration it gives following error dialog:

        line 884 of /lib/blocklib.php: coding_exception thrown
        * line 986 of /lib/blocklib.php: call to block_manager->check_region_is_known()
        * line 303 of /lib/blocklib.php: call to block_manager->ensure_instances_exist()
        * line 87 of /lib/ajax/blocks.php: call to block_manager->get_blocks_for_region()
        
        Show
        Rajesh Taneja added a comment - Sorry Ruslan, It works fine in master, but in 23 it is not working for both RTL and LTR. Please see attached video. Also, in RTL on course settings page (There are no blocks on left side.), but user can D-n-D blocks on left side. In Stable there was no error, but refresh page put moved block on right bottom. In Integration it gives following error dialog: line 884 of /lib/blocklib.php: coding_exception thrown * line 986 of /lib/blocklib.php: call to block_manager->check_region_is_known() * line 303 of /lib/blocklib.php: call to block_manager->ensure_instances_exist() * line 87 of /lib/ajax/blocks.php: call to block_manager->get_blocks_for_region()
        Hide
        Eloy Lafuente (stronk7) added a comment -

        pong?

        Show
        Eloy Lafuente (stronk7) added a comment - pong?
        Hide
        Frédéric Massart added a comment -

        I haven't written the patch, but looking at it I don't think this is related as only a JS file has been modified, and the error is from PHP.

        Show
        Frédéric Massart added a comment - I haven't written the patch, but looking at it I don't think this is related as only a JS file has been modified, and the error is from PHP.
        Hide
        Frédéric Massart added a comment -

        Oh I know what the issue should be... right_to_left() does not exist in 2.3. It's been introduced in 2.4 onwards (see my comment above). I should have looked at the patch for 2.3 before pushing for integration. Sorry!

        Show
        Frédéric Massart added a comment - Oh I know what the issue should be... right_to_left() does not exist in 2.3. It's been introduced in 2.4 onwards (see my comment above). I should have looked at the patch for 2.3 before pushing for integration. Sorry!
        Hide
        Eloy Lafuente (stronk7) added a comment -

        Ah, that thought was in my mind yesterday (at bed) but was lazy to wakeup to write about it, hehe.

        If we can switch to old rtl detection in the 23_STABLE branch that would be AWESOME!

        Show
        Eloy Lafuente (stronk7) added a comment - Ah, that thought was in my mind yesterday (at bed) but was lazy to wakeup to write about it, hehe. If we can switch to old rtl detection in the 23_STABLE branch that would be AWESOME!
        Hide
        Frédéric Massart added a comment -

        Here is a patch:
        git://github.com/FMCorz/moodle.git
        MDL-37664-23-int
        https://github.com/FMCorz/moodle/commit/4174fc117635bc075f5c9ed75396591411dca869

        Show
        Frédéric Massart added a comment - Here is a patch: git://github.com/FMCorz/moodle.git MDL-37664 -23-int https://github.com/FMCorz/moodle/commit/4174fc117635bc075f5c9ed75396591411dca869
        Hide
        Eloy Lafuente (stronk7) added a comment -

        has perfect sense, thanks! pushed! reseting this to another testing round for 23_STABLE (assuming all problems were 23_STABLE only).

        Ciao

        Show
        Eloy Lafuente (stronk7) added a comment - has perfect sense, thanks! pushed! reseting this to another testing round for 23_STABLE (assuming all problems were 23_STABLE only). Ciao
        Hide
        Rajesh Taneja added a comment -

        Thanks Fred and Eloy,

        Works fine on 23 now.
        Passing...

        Show
        Rajesh Taneja added a comment - Thanks Fred and Eloy, Works fine on 23 now. Passing...
        Hide
        Damyon Wiese added a comment -

        This issue has been integrated upstream and is now available via git (and in some hours, via mirrors and downloads).

        Thanks for your contributions!

        Show
        Damyon Wiese added a comment - This issue has been integrated upstream and is now available via git (and in some hours, via mirrors and downloads). Thanks for your contributions!
        Hide
        Eloy Lafuente (stronk7) added a comment -

        Thanks Rajesh, I was going to retest this yesterday myself, but felt slept before it (literally)! Cheers!

        Show
        Eloy Lafuente (stronk7) added a comment - Thanks Rajesh, I was going to retest this yesterday myself, but felt slept before it (literally)! Cheers!

          People

          • Votes:
            0 Vote for this issue
            Watchers:
            5 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved: