Problem/Motivation
If you create a new theme and don't define your own regions, it will inherit the default regions defined in core/lib/Drupal/Core/Extension/ThemeHandler.php. These regions are defined in a seemingly random order, so that e.g. header is below content, and footer is in the middle of the list:
This makes it hard to visually scan for the region you need, and is pretty much guaranteed to not reflect the rendered order of the regions.
Proposed resolution
Re-order the regions to follow generally accepted practices. Obviously this is somewhat subjective, but I think we can all agree on some sane defaults, such as header being at the top, and footer being at the bottom.
Remaining tasks
Review patch.
User interface changes
Display of block regions on admin/structure/block/list will change, but this should be a strictly visual administrative change- underlying code and rendered output will not change.
API changes
None
Data model changes
None
Comment | File | Size | Author |
---|---|---|---|
#34 | 2603598-34.patch | 4.04 KB | anya_m |
default-regions.png | 99.29 KB | Dane Powell |
Comments
Comment #2
Dane Powell CreditAttribution: Dane Powell at Acquia commentedThis patch adjusts the default order to roughly match Bartik's:
Header
Primary menu
Secondary menu
Page top
Page bottom
Highlighted
Breadcrumb
Help
Content
Sidebar first
Sidebar second
Footer
Comment #4
Dane Powell CreditAttribution: Dane Powell at Acquia commentedUgh, so this caused the block tests to fail. This is because they work by hardcoding the index of the table row in the block admin ui corresponding to a certain region, and then checking that the block exists at that row (barf).
This patch fixes that by instead looking for the block and then confirming that its region is set correctly- no hardcoding of magic numbers.
Comment #5
Dane Powell CreditAttribution: Dane Powell at Acquia commentedComment #6
joelpittetI hope this can get into 8.0.1 or something.
Comment #7
star-szrJust want to say thanks for this! We can probably get it in after release in an 8.0.x version or 8.1.x version, doesn't seem to be disruptive at all.
Comment #8
Dane Powell CreditAttribution: Dane Powell at Acquia commentedIf you can review the patch and mark this issue as Reviewed and Tested by the Community, that would go a long way towards getting it into core.
Comment #9
joelpittetYes the order is much better. #mobilefirst
Comment #10
NaheemSays CreditAttribution: NaheemSays commentedIn past theming for Drupal 7, I have noticed that when enabling and switching themes, blocks from regions that no longer exist in the new theme are placed in the first region listed.
Often this is the header and this can often make the site temporarily unuseable.
I do not know if this is still the case with Drupal 8, but when listing regions, this would be useful to consider (and potentially list the content region first as it is generally the most adaptable to handle generic blocks).
Comment #11
alexpott#10 is entirely correct - see block_theme_initialize() - it still exists in D8. So we definitely need to think about how the result of system_default_region() is worked out. I don't think that the current behaviour is correct (sidebar_first) and I don't think the behaviour with the current patch is correct either.
Comment #12
Dane Powell CreditAttribution: Dane Powell commented@alexpott what would you prefer, placing the content region first, or modifying block_theme_initialize() to place blocks into the content region if available, instead of the first available region?
Comment #13
alexpott@Dane Powell well the issue is the
system_default_region()
logic. OTOH it has behaved this way at least since Drupal 7. The default region in Bartik for both 7 and 8 is header. The problem with this change is that is changing the result ofsystem_default_region()
for themes with no regions. I think what we should do here is move this 8.1.x and proceed but also file a follow up to allow themes to declare a different default region from the first region. As this will change the default region for themes which don't declare any regions I think we need a very small CR for the new behaviour.Comment #14
Dane Powell CreditAttribution: Dane Powell commentedOkay, so we all agree that the current method of picking the default region is ridiculous. But can we please not let that get in the way of committing this relatively unrelated patch?
Before the patch, the default region for themes not defining their own would be sidebar_first, which is pretty ridiculous.
After the patch, the default region is header, which is equally ridiculous.
So technically, yes this is a change, but such a trivial one that I don't see the need for a CR, and the whole default region picking system should be revamped as a follow-up ticket.
Comment #15
Dane Powell CreditAttribution: Dane Powell commentedComment #16
Dane Powell CreditAttribution: Dane Powell as a volunteer commentedHere's the follow-up issue: #2635166: block_theme_initialize() falls back to putting blocks in the first region it finds, provide a better default
Does this still need a change record?
Comment #17
joachim CreditAttribution: joachim commentedI think we could fix this on 8.0.x if:
- we fix #2635166: block_theme_initialize() falls back to putting blocks in the first region it finds, provide a better default, making the new theme property optional
- as part of that, we set the default region to 'header' for themes that don't define regions and don't define a default region (the default default ;), at least on 8.0.x. For 8.1.x, regionless themes can maybe have 'content' as a better default? But that's a minor point
- we then fix this issue, and themes that don't define regions continue to have the same default region.
Comment #18
joachim CreditAttribution: joachim commentedHmm nope, what am I saying? That's not quite right -- #2635166: block_theme_initialize() falls back to putting blocks in the first region it finds, provide a better default is only for 8.1.x, as it adds new functionality.
But what we can do on 8.0.x is preserve the default region that's returned for themes that do not specify any regions, like this:
It's not very pretty, but it allows us to fix this UX bug on 8.0.x. For 8.1.x, we can change the behaviour and provide a CR.
Comment #19
joachim CreditAttribution: joachim commentedUpdated patch that includes my suggestion from above.
This wouldn't need a change record, as the default region behaviour is unchanged. As such, we can fix this on 8.1.x and also on 8.0.x.
Once this bug is fixed, we can implement #2635166: block_theme_initialize() falls back to putting blocks in the first region it finds, provide a better default on 8.1.x. As part of that, we'd probably want to clean up clean up the special case handling added in this patch, and thus at that point do a CR for the behaviour change.
Comment #20
NaheemSays CreditAttribution: NaheemSays commentedIs there any guarantee that sidebar_first will exist?
Comment #21
joelpittetre #20 I don't think so as the defaults are unioned so if defined in our theme it will override the defaults. More likely 'content' will exist... but still it may not.
Comment #22
joachim CreditAttribution: joachim commented> Is there any guarantee that sidebar_first will exist?
Yes there is. Because we only do this for the themes that do not define ANY regions in their theme.info file. Therefore they get exactly the list of defaults from ThemeHandler::rebuildThemeData().
Does the code need a bit more commenting to explain this more clearly?
Comment #23
joelpittet@joachim is there a reason it fallback to
sidebar_first
and notcontent
when the theme doesn't provide any? And shouldn't the ActiveTheme hold that information in the registry instead of having to parse the info yaml again?Comment #24
dcrocks CreditAttribution: dcrocks as a volunteer commentedNot sure why you need to special case themes that don't include region definitions in their info.yml. The way I read the code they will get exactly the default region list. Stark, for example, doesn't specify any regions, yet when you install it it has exactly the default region list. And since sidebar_first is currently 1st in that list, it will be the default region for Stark.
Comment #25
dcrocks CreditAttribution: dcrocks as a volunteer commentedOne possible argument for the current ordering of the list is that the default region will be sidebar_first. If it were the header then a bunch of blocks that would be inappropriate for the header would show up there. For example, for stark, with the new list the 'powered_by' and 'footer' blocks would appear in the header. No matter what list is used any admin will immediately clean all that up. It is my impression from the issue summary the only purpose for this issue is that the admin block layout UI not look so unnatural for new themes without any region designations in their info.yml. Maybe sidebar_first should be left on top.
Comment #26
joachim CreditAttribution: joachim commented> is there a reason it fallback to sidebar_first and not content when the theme doesn't provide any?
Yes, to preserve the current default region behaviour, so this bug can be fixed on 8.0.x as well as 8.1.x.
> And shouldn't the ActiveTheme hold that information in the registry instead of having to parse the info yaml again?
AFAICT, if we look at ActiveTheme we will get the actual list of regions, and for a theme that doesn't specify any that will be the default list. So in that case, we can't tell if it was a theme with no regions in its info.yml, or a theme that just happened to define the same list itself.
Comment #32
markhalliwellComment #34
anya_m CreditAttribution: anya_m as a volunteer and at Skilld commentedRerolled it
Comment #35
alexpott@anya_m thanks for working on this issue.
This shouldn't be done here. Doing it potentially means unnecessary file system reads. Maybe we could calculate the default region in ThemeHandler::rebuildThemeData().
We also need a test the special case scenario.
Comment #37
andypostI think the related should came in first