When enabling or repositioning a block by using the select dropdown for blocks at admin/structure/blocks, the magic javascript zips it to the top of the selected region. Which is great, except that after saving the block admin page form, you find that the new block has actually been put at the bottom of the region, and that is where it displays when viewing the site, not the top.
Comment | File | Size | Author |
---|---|---|---|
#61 | core-js-block-admin-1039666-61.patch | 1.23 KB | dozymoe |
#56 | core-js-block-admin-1039666-56.patch | 1.4 KB | Kevin Morse |
#53 | core-js-block-admin-1039666-52.patch | 1.41 KB | nod_ |
#45 | 1039666-block-admin-page-45.patch | 1.42 KB | Kevin Morse |
#44 | 1039666-block-admin-page-41.patch | 1.42 KB | droplet |
Comments
Comment #1
yoroy CreditAttribution: yoroy commentedConfirmed. In blocks ui, if you use the select list to move blocks into a region, it is shown inserted on top but after saving and viewing in the front-end, the block is shown last in the region. Drag-dropping works, happens only when using the select list.
Honoring the insert-on-top make most sense to me because it will show the region name directly above it, giving you direct confirmation you put it in the right region.
Comment #2
droplet CreditAttribution: droplet commented#1308982: wrong sorting when assigning block to a region
Comment #3
Jelle_SGave a try at writing a patch for this :-)
Comment #4
Jelle_Sforgot the status
Comment #5
xjmThe patch looks good codewise. Note that the Drupal 8.x patch will need to be rerolled, because the core directory structure for Drupal 8 has now changed. (For more information, see #22336: Move all core Drupal files under a /core folder to improve usability and upgrades). When the patch has been rerolled, please set the issue back to "Needs Review."
Tagging as novice for the task of rerolling the Drupal 8.x patch.
If you need help rerolling this patch, you can come to core office hours or ask in #drupal-gitsupport on IRC.
Also, it would be good to get a couple users testing this manually in different browsers and confirming the issue is resolved as expected.
Comment #6
Jelle_Swill reroll patch asap :-)
Comment #7
Jelle_Srerolled patch
Comment #8
good_man CreditAttribution: good_man commentedWorks fine on webkit + firefox. Guess this is the best way in the patch, since the DOM of blocks page is tricky.
Comment #9
joachim CreditAttribution: joachim commentedConfirming this fixes the problem for me too.
Hurrah! This has been a real niggle on D7 - tagging for backport.
Comment #10
catchOK patches like this make me wish we had automated JavaScript testing, but given this has been manually tested and it's a small change, committed/pushed to 8.x.
Will need a quick re-roll for 7.x.
Comment #11
Jelle_Srerolled patch for drupal 7
Comment #12
Devin Carlson CreditAttribution: Devin Carlson commentedThe patch makes the same modifications and applies cleanly. I tested with a fresh version of Drupal 7 and after applying the patch blocks moved using the selection list now appear at the bottom of the selected region as they should.
Comment #13
good_man CreditAttribution: good_man commentedThis bug is also in D6. Let's fix it while here :) one problem is nextUntil() is only in jQuery >= 1.4, and D6 core is using 1.2.6. So better finding another way to solve it.
Comment #14
Jelle_SIt is a bit of a workaround, but this patch seems to work:
Comment #15
Jelle_Serr, about the status of this issue, should this be RTBC because of the D7 patch or needs review for the D6 patch, I'll put it back to RTBC so we can get the D7 patch in first?
Comment #17
Jelle_Spatch failed to apply because it tried to apply to D7 branch. Re-test once the D7 patch gets in...
Comment #18
Jelle_SComment #19
droplet CreditAttribution: droplet commentedtestbot failed.
Comment #20
droplet CreditAttribution: droplet commentedahh #11 is D7
failed patch is D6
Comment #21
good_man CreditAttribution: good_man commentedIs this change in behavior required for fixing the bug? can't we fix it without making changing the default order from first to last?
Anyhow patch in #14 tested manually and working, but note the point I mentioned the line above.
P.S. put a -D6 at the end of the patch name to stop testbot from testing it.
Powered by Dreditor.
Comment #22
Jelle_SThat is the exact bug we're trying to fix?
I can't edit the patch name anymore now but I will remember this for the future :-)
Comment #23
good_man CreditAttribution: good_man commentedThe actual bug is, when you select blocks they got sorted on top, when you save they got missed up. The order top-first (or as you edited it bottom-first) is not as important as when saving I get different sort than the one I saw when I selected the blocks from their region-selectbox.
Comment #24
droplet CreditAttribution: droplet commentedPHP coding is correct, JS errors. mark #11 RTBC.
Comment #25
webchickCommitted and pushed to 7.x. Thanks!
Marking down to 6.x.
Comment #26
Jelle_S#14: drupal6-1039666-14.patch queued for re-testing.
Comment #27
VM CreditAttribution: VM commentedCould I bother others to manually retest this patch for D7.x again. The patch in #11 seems to have broken the blocks admin screen.
A fresh install of D7-dev rolled 12/25/2011, disabled blocks are no longer able to be set to any empty region. They get pushed to the bottom of the disabled section. Upon trying to set them again, they disappear. I have narrowed it down to this change (the change in #11). When I overwrite the -dev version of block.js with a version of the file from 7.10 the block screen works again albeit with the original issue the patch was trying to fix.
This is occurring in at least Firefox8 and IE8. I've yet to test other browsers.
Steps to reproduce:
go to administer -> structure -> blocks
try setting a disabled block to an empty region
the block gets put at the bottom of the disabled blocks list
try setting it to an empty region again
block disappears from block administration screen
Comment #28
VM CreditAttribution: VM commentedremarking as active.
Comment #29
VM CreditAttribution: VM commentederp and shifting back to 7.x to get eyes back on it.
Comment #30
Devin Carlson CreditAttribution: Devin Carlson commentedI just checked out a fresh copy of D7 via Git.
I am able to successfully set a block's region by using either the draggable handle or the selection list. When using the list to enable a block, the block's table row is correctly moved to the bottom of the selected region and when using the list to disable a block, the block's table row is correctly positioned at the bottom of the disabled region.
Are you still having this problem with a fresh checkout of D7 and can you post the steps to reproduce? I can help test.
Comment #31
yoroy CreditAttribution: yoroy commentedI couldn't reproduce either.
Comment #32
droplet CreditAttribution: droplet commentedFailed assign to an empty block region (No blocks in this region)
Comment #33
VM CreditAttribution: VM commentedyes issue is still reproduceable in the 7.x-dev from 12/28
droplet is correct. The issue isn't with blocks which are being assigned to regions which already have blocks in them. The issue is when a block is being assigned to a region that doesn't yet have any blocks assigned to it.
steps to reproduce.
in the disabled blocks list set a block using the select list to a region which does not have a block in it.
The block is sent to the bottom of the disabled blocks list.
the expected behavior would be that the block is put in the region the was selected.
Comment #34
dcrocks CreditAttribution: dcrocks commentedCannot reproduce in 7.x dev(12/15) or drupal 8.x dev. Can you give more info about environment and block/region?
Comment #35
dcrocks CreditAttribution: dcrocks commentedDid a clean install of 7.x dev(12/28). Changed nothing and went straight to admin/structure/blocks and was able to reproduce. Not only that, can't move a block currently assigned to a region to a new(empty) region. Can move a block currently assigned to a region to a region that has blocks assigned to it. Something definitely wrong. Running on OS X using sqllite for database and bartik for theme.
Comment #36
webchickWow, thanks for testing and catching this!!
Ok, reverted these commits:
7.x-dev and 8.x-dev should be back working again.
Comment #37
VM CreditAttribution: VM commentedHeh, and I had made a video to convey what I couldn't seem to in text! FIGURES!
I'm attaching the link to the vid anyway for those who couldn't reproduce or those who land here and don't understand what was happening
http://youtu.be/vL_RFpFoVZI
Comment #38
dcrocks CreditAttribution: dcrocks commentedBuilt 7.x from git and tested. Can assign blocks to empty regions with no problems. So issue needs work.
Comment #39
droplet CreditAttribution: droplet commentedfixes and cleanup.
Each block allowed assign to one region only. I see no reason not to select it directly.
// Manually update weights and restripe.
I do not know why we add these codes. tabledrag.js handle these changes.
nextUntil in v1.7 is different:
http://api.jquery.com/nextUntil/
Comment #40
droplet CreditAttribution: droplet commentedevent never used.
it's always ONE selected row
it can be selected directly.
redundant codes.
0 days to next Drupal core point release.
Comment #41
droplet CreditAttribution: droplet commentedreroll
Comment #42
caelon CreditAttribution: caelon commentedTested and did not appear to work in 8. Steps:
As an aside, I was not able to replicate #37.
My first patch review so feel free to comment on ways to improve this report.
Comment #43
caelon CreditAttribution: caelon commentedRemoved Needs manual testing tag since it needs a rework. Sorry for the extra post but did not see a way to edit previous one while removing tag.
Comment #44
droplet CreditAttribution: droplet commentedThanks @caelon.
are you sure cleanly apply the patch ? can you try once more, thanks.
Comment #45
Kevin Morse CreditAttribution: Kevin Morse commentedPatch doesn't apply anymore due to some small changes to jQuery selectors.
I updated #41 to work with the changes and have the following behaviour on a fresh D8 install.
Moving to already populated region:
1. Move Syndicate to bottom of Sidebar first (4/4) or choose Sidebar first in drop down
2. Syndicate is at bottom of list.
3. Save Blocks and Syndicate is still at bottom of list
4. Check front page and Syndicate is at the bottom!
Moving to empty region.
1. Move Syndicate to empty region
2. Syndicate appears there.
3. Save Blocks and Syndicate is there.
Here is the patch I used for this round of testing. It should apply to the latest D8.
Comment #46
droplet CreditAttribution: droplet commentedSorry ??
what's the different between #44 & #45 ?
Comment #47
droplet CreditAttribution: droplet commentedComment #48
Kevin Morse CreditAttribution: Kevin Morse commentedLine 10
-- $('tr.region-message', table).each(function () {
+- table.find('tr.region-message').each(function () {
Line 24
-- $(row).addClass('drag-previous');
+- row.addClass('drag-previous');
#44 wouldn't apply to the latest D8.
Comment #49
droplet CreditAttribution: droplet commentedThanks. can you patch Line 10 to latest version.
Comment #50
Kevin Morse CreditAttribution: Kevin Morse commentedLine 10 should be at the latest version now.
This patch should be ready to go.
Comment #51
Kevin Morse CreditAttribution: Kevin Morse commented#45: 1039666-block-admin-page-45.patch queued for re-testing.
Comment #52
droplet CreditAttribution: droplet commentedThanks,
after JS clean up, following line
should be:
Comment #53
nod_There were a few redundant things.
here it is a little cleaned up. RTBC otherwise, works fine for me, please confirm.
Comment #54
Kevin Morse CreditAttribution: Kevin Morse commentedWorks as expected for me. I did find a new problem on a clean installation if one uses the drop down to move a block to a pre-populated region such as Sidebar First.
Upon selecting Sidebar first with the dropdown Syndicate moves to the bottom of the Sidebar first region (fourth position). However, after saving Syndicate appears to be third (Search form, Navigation, Syndicate, User login) this is because the last three blocks all have a weight of 0 while Search form has a weight of -1 so the last three blocks are sorted alphabetically. Weights are (-1, 0, 0, 0) and stay at 0 until one uses the handle to move a block into the region.
I think that should probably be a separate issue though as it is related to the blocks default weightings not js.
Therefore this is RTBC!
Comment #55
catchThanks! Committed/pushed to 8.x, moving to 7.x for backport.
Comment #56
Kevin Morse CreditAttribution: Kevin Morse commentedThis should do it.
Tested with Chrome and Firefox and the behaviour is working as it should be.
Comment #57
Niklas Fiekas CreditAttribution: Niklas Fiekas commentedI confirmed the backport is straight forward (or backward :D) related to the original 8.x patch and that it indeed works in Chrome and Internet Explorer. Thank you @Kevin Morse!
Comment #58
webchickWow, that's quite the clean-up. Thanks.
Committed and pushed to 7.x.
Comment #60
cweagansFixing tags per http://drupal.org/node/1517250
Comment #61
dozymoe CreditAttribution: dozymoe commentedI think this is a continuation of this issue, of the last patch at least.
Found bug, when there is no block in the last region (that would be the Disabled region), you can't move a block to that region using the
select
drop-down.Steps to reproduce
select
drop-down.Other notes
Also other findings:
tableDrag.row()
expects first argument to be DOM element instead of jQuery object, I think.checkEmptyRegions()
expects the second argument to be an instance oftableDrag.row()
.$(this)
that could be optimized as$this = $(this);
I think, but I didn't dare to make the changes (maybe jQuery has a cache system, or the call isn't expensive, dunno).Comment #62
czigor CreditAttribution: czigor commentedI think a new issue should be created. See #2177335: Selecting 'Disabled' does not move the block to the disabled region when there are no disabled blocks.
Comment #63
droplet CreditAttribution: droplet commentedStarted from #61 it seems like a new bug. This isn't a duplicated issue. I fixed the status.
Comment #64
xjm