#322 | interdiff.txt | 886 bytes | Wim Leers |
#322 | 2476947-322-convert-page-title-block.patch | 82.2 KB | Wim Leers |
|
#318 | interdiff-315-318.txt | 540 bytes | RainbowArray |
#318 | 2476947-318-convert-page-title-block.patch | 81.42 KB | RainbowArray |
|
#315 | 2476947-315-convert-page-title-block.patch | 81.42 KB | RainbowArray |
|
#310 | 2476947-310-convert-page-title-block.patch | 81.42 KB | RainbowArray |
|
#301 | interdiff-2476947-293-301.txt | 3.38 KB | RainbowArray |
#301 | 2476947-301-convert-page-title-block.patch | 81.36 KB | RainbowArray |
|
#293 | interdiff-2476947-288-293.txt | 2.41 KB | RainbowArray |
#293 | 2476947-293-convert-page-title-block.patch | 81.34 KB | RainbowArray |
|
#288 | interdiff-2476947-285to288.txt | 1.31 KB | davidhernandez |
#288 | convert_title_page-2476947-288.patch | 81.47 KB | davidhernandez |
|
#285 | 2476947-285-convert-page-title-block.patch | 80.8 KB | RainbowArray |
|
#283 | interdiff.txt | 3.59 KB | star-szr |
#283 | convert_title_page-2476947-283.patch | 80.81 KB | star-szr |
|
#279 | interdiff-2476947-268-278.txt | 560 bytes | RainbowArray |
#279 | 2476947-278-convert-title-block.patch | 80.82 KB | RainbowArray |
|
#274 | Screenshot 2015-09-19 21.04.52.jpg | 267.83 KB | LewisNyman |
#274 | Appearance___Site-Install.png | 593.67 KB | LewisNyman |
#274 | Appearance___Site-Install.png | 589 KB | LewisNyman |
#271 | interdiff-2476947-260-268.txt | 562 bytes | RainbowArray |
#271 | 2476947-268-convert-page-title-block.patch | 80.27 KB | RainbowArray |
|
#262 | TitleBlockInSeven.png | 9.33 KB | mgifford |
#260 | interdiff-2476947-257-260.txt | 1.17 KB | RainbowArray |
#260 | 2476947-260-convert-page-title-block.patch | 80.27 KB | RainbowArray |
|
#257 | interdiff-2476947-253-257.txt | 1.16 KB | RainbowArray |
#257 | 2476947-257-convert-page-title-block.patch | 81.06 KB | RainbowArray |
|
#253 | interdiff-2476947-247-252.txt | 2.06 KB | RainbowArray |
#253 | 2476947-252-convert-page-title-block.patch | 81.47 KB | RainbowArray |
|
#248 | interdiff-2476947-245-247.txt | 18.24 KB | RainbowArray |
#248 | 2476947-247-convert-page-title-block.patch | 80.08 KB | RainbowArray |
|
#245 | interdiff.txt | 7.37 KB | Wim Leers |
#245 | convert_page_title_to_block-2476947-245.patch | 61.84 KB | Wim Leers |
|
#236 | interdiff-2476947-236-236FAIL.txt | 640 bytes | RainbowArray |
#236 | interdiff-2476947-235-236.txt | 934 bytes | RainbowArray |
#236 | 2476947-236-convert-page-title-block.patch | 62.23 KB | RainbowArray |
|
#236 | 2476947-236-convert-page-title-block-FAIL.patch | 62.86 KB | RainbowArray |
|
#235 | interdiff.txt | 3.94 KB | Wim Leers |
#235 | convert_page_title_to_block-2476947-235.patch | 61.61 KB | Wim Leers |
|
#231 | interdiff-2476947-230-231.txt | 2.06 KB | RainbowArray |
#231 | 2476947-231-convert-page-title-block.patch | 61.1 KB | RainbowArray |
|
#230 | interdiff.txt | 8.42 KB | Wim Leers |
#230 | convert_page_title_to_block-2476947-230.patch | 60.35 KB | Wim Leers |
|
#228 | interdiff-2547159-218-228.txt | 640 bytes | RainbowArray |
#228 | 2547159-228-convert-page-title-block.patch | 62.29 KB | RainbowArray |
|
#226 | Screen Shot 2015-09-14 at 10.31.16 PM.png | 87.83 KB | RainbowArray |
#226 | Screen Shot 2015-09-14 at 10.25.33 PM.png | 124.95 KB | RainbowArray |
#224 | bartik-blockdisabled.png | 130.17 KB | davidhernandez |
#223 | linkscreenshot.png | 9.21 KB | davidhernandez |
#218 | interdiff.txt | 1.07 KB | Wim Leers |
#218 | convert_page_title_to_block-2476947-218.patch | 62.92 KB | Wim Leers |
|
#212 | interdiff.txt | 5.93 KB | Wim Leers |
#212 | convert_page_title_to_block-2476947-211.patch | 63.87 KB | Wim Leers |
|
#212 | convert_page_title_to_block-2476947-211-FAIL.patch | 65.83 KB | Wim Leers |
|
#210 | xhprof runs.zip | 227.52 KB | Wim Leers |
#210 | ab runs.zip | 7.51 KB | Wim Leers |
#210 | histogram_interleaved.png | 10.92 KB | Wim Leers |
#206 | interdiff.txt | 2.44 KB | Wim Leers |
#206 | convert_page_title_to_block-2476947-206.patch | 63 KB | Wim Leers |
|
#204 | interdiff-2476947-201-204.txt | 669 bytes | RainbowArray |
#204 | 2476947-204-convert-page-title-block.patch | 61.67 KB | RainbowArray |
|
#201 | interdiff-2476947-200-201.txt | 2.86 KB | RainbowArray |
#201 | 2476947-201-convert-page-title-block.patch | 61.64 KB | RainbowArray |
|
#200 | interdiff-2476947-180-200.txt | 5.68 KB | RainbowArray |
#200 | 2476947-200-convert-page-title-block.patch | 61.53 KB | RainbowArray |
|
#198 | install-configure.png | 263.65 KB | davidhernandez |
#197 | runningupdateafterpatch.png | 217.41 KB | davidhernandez |
#197 | prepatch-install.png | 189.65 KB | davidhernandez |
#196 | after-install.png | 79.47 KB | RainbowArray |
#196 | before-install.png | 65.66 KB | RainbowArray |
#192 | bartik-maintenance.png | 82.84 KB | davidhernandez |
#192 | bartik-before-maintenance.png | 97.31 KB | davidhernandez |
#189 | install-title.png | 135.59 KB | davidhernandez |
#189 | bartik-title.png | 156.39 KB | davidhernandez |
#189 | seven-title.png | 183.95 KB | davidhernandez |
#189 | classy-node-title.png | 143.74 KB | davidhernandez |
#189 | stark-node-title.png | 100.62 KB | davidhernandez |
#180 | interdiff.txt | 2.99 KB | Wim Leers |
#180 | convert_page_title_to_block-2476947-180.patch | 61.37 KB | Wim Leers |
|
#174 | interdiff.txt | 7.11 KB | Wim Leers |
#174 | convert_page_title_to_block-2476947-174.patch | 60.04 KB | Wim Leers |
|
#172 | interdiff.txt | 21.59 KB | Wim Leers |
#172 | convert_page_title_to_block-2476947-172.patch | 59.63 KB | Wim Leers |
|
#171 | interdiff.txt | 3.71 KB | Wim Leers |
#171 | convert_page_title_to_block-2476947-171.patch | 78.83 KB | Wim Leers |
|
#170 | interdiff.txt | 8.22 KB | Wim Leers |
#170 | convert_page_title_to_block-2476947-170.patch | 78.73 KB | Wim Leers |
|
#169 | interdiff.txt | 1.45 KB | Wim Leers |
#168 | interdiff.txt | 8.07 KB | Wim Leers |
#168 | convert_page_title_to_block-2476947-168.patch | 83.93 KB | Wim Leers |
|
#167 | interdiff.txt | 8.07 KB | Wim Leers |
#167 | convert_page_title_to_block-2476947-167.patch | 85.27 KB | Wim Leers |
|
#165 | interdiff-2476947-163-164.txt | 11.34 KB | RainbowArray |
#165 | 2476947-164-convert-page-title-block.patch | 84.89 KB | RainbowArray |
|
#164 | interdiff-2476947-163-164.txt | 11.34 KB | RainbowArray |
#164 | 2476947-164-convert-page-title-block.patch | 84.89 KB | RainbowArray |
|
#163 | interdiff-2476947-162-163.txt | 10.95 KB | RainbowArray |
#163 | 2476947-163-convert-page-title-block.patch | 75.61 KB | RainbowArray |
|
#162 | interdiff-converttitle-156to162.txt | 663 bytes | davidhernandez |
#162 | convert_title_page-2476947-162.patch | 75.68 KB | davidhernandez |
|
#156 | interdiff.txt | 2.78 KB | Wim Leers |
#156 | convert_page_title_to_block-2476947-156.patch | 75.03 KB | Wim Leers |
|
#155 | interdiff.txt | 1007 bytes | Wim Leers |
#155 | convert_page_title_to_block-2476947-155.patch | 73.52 KB | Wim Leers |
|
#147 | interdiff.txt | 6.51 KB | Wim Leers |
#147 | convert_page_title_to_block-2476947-145.patch | 72.66 KB | Wim Leers |
|
#144 | screenshot-converttitle.png | 40.14 KB | davidhernandez |
#144 | interdiff-pagetitle-134to144.txt | 1.69 KB | davidhernandez |
#144 | convert_title_page-2476947-144.patch | 67.7 KB | davidhernandez |
|
#134 | interdiff.txt | 2.96 KB | Wim Leers |
#134 | convert_page_title_to_block-2476947-134.patch | 66.8 KB | Wim Leers |
|
#133 | interdiff.txt | 930 bytes | Wim Leers |
#133 | convert_page_title_to_block-2476947-133.patch | 66.92 KB | Wim Leers |
|
#130 | interdiff.txt | 1.77 KB | Wim Leers |
#130 | convert_page_title_to_block-2476947-130.patch | 66.92 KB | Wim Leers |
|
#129 | interdiff.txt | 1.84 KB | Wim Leers |
#129 | convert_page_title_to_block-2476947-129.patch | 66.68 KB | Wim Leers |
|
#126 | solution-b-interdiff.txt | 10.98 KB | Wim Leers |
#126 | solution-a-interdiff.txt | 3.57 KB | Wim Leers |
#126 | interdiff.txt | 1.07 KB | Wim Leers |
#126 | convert_page_title_to_block-2476947-126.patch | 65.47 KB | Wim Leers |
|
#123 | interdiff.txt | 1.61 KB | Wim Leers |
#123 | convert_page_title_to_block-2476947-123.patch | 64.83 KB | Wim Leers |
|
#116 | interdiff-2476947-113-116.txt | 1.67 KB | RainbowArray |
#116 | 2476947-116-convert-page-title-block.patch | 65.71 KB | RainbowArray |
|
#113 | interdiff.txt | 2.05 KB | joelpittet |
#113 | convert_title_page-2476947-113.patch | 65.63 KB | joelpittet |
|
#107 | interdiff-2476947-98-107.txt | 7.52 KB | RainbowArray |
#107 | 2476947-107-convert-page-title-block.patch | 66.53 KB | RainbowArray |
|
#98 | convert_title_page-2476947-98.patch | 72.52 KB | lauriii |
|
#95 | convert_title_page-2476947-94.patch | 66.33 KB | lauriii |
|
#95 | interdiff.txt | 961 bytes | lauriii |
#91 | convert_title_page-2476947-91.patch | 66.5 KB | lauriii |
|
#85 | interdiff-2476947-76-reroll-85.txt | 1.62 KB | RainbowArray |
#85 | 2476947-85-convert-page-title-block.patch | 67.09 KB | RainbowArray |
|
#84 | 2476947-76-reroll.patch | 66.86 KB | RainbowArray |
|
#76 | interdiff.txt | 1.44 KB | Wim Leers |
#76 | 2476947-76-convert-page-title-block.patch | 67.13 KB | Wim Leers |
|
#75 | interdiff.txt | 1.45 KB | Wim Leers |
#75 | 2476947-75-convert-page-title-block.patch | 67.13 KB | Wim Leers |
|
#74 | interdiff.txt | 1.67 KB | Wim Leers |
#74 | 2476947-74-convert-page-title-block.patch | 65.79 KB | Wim Leers |
|
#73 | interdiff.txt | 15.57 KB | Wim Leers |
#73 | 2476947-73-convert-page-title-block.patch | 65.32 KB | Wim Leers |
|
#70 | interdiff.txt | 1.47 KB | Wim Leers |
#70 | 2476947-70-convert-page-title-block.patch | 50.67 KB | Wim Leers |
|
#68 | interdiff.txt | 8.64 KB | Wim Leers |
#68 | 2476947-68-convert-page-title-block.patch | 49.32 KB | Wim Leers |
|
#67 | interdiff.txt | 7.9 KB | Wim Leers |
#67 | 2476947-67-convert-page-title-block.patch | 46.28 KB | Wim Leers |
|
#64 | interdiff-2476947-62-64.txt | 13.98 KB | RainbowArray |
#64 | 2476947-64-convert-page-title-block.patch | 41.61 KB | RainbowArray |
|
#62 | interdiff-2476947-59-62.txt | 619 bytes | RainbowArray |
#62 | 2476947-62-convert-page-title-block.patch | 27.63 KB | RainbowArray |
|
#59 | interdiff.txt | 5.68 KB | Wim Leers |
#59 | 2476947-59-convert-page-title-block.patch | 27.02 KB | Wim Leers |
|
#54 | interdiff-2476947-52-54.txt | 623 bytes | RainbowArray |
#54 | 2476947-54-convert-page-title-block.patch | 26.04 KB | RainbowArray |
|
#52 | interdiff-2476947-50-52.txt | 1.64 KB | RainbowArray |
#52 | 2476947-52-convert-page-title-block.patch | 26.65 KB | RainbowArray |
|
#50 | interdiff-2476947-48-50.txt | 632 bytes | RainbowArray |
#50 | 2476947-50-convert-page-title-block.patch | 26.62 KB | RainbowArray |
|
#48 | interdiff-2476947-43-48.txt | 1.8 KB | RainbowArray |
#48 | 2476947-48-convert-page-title-block.patch | 26.61 KB | RainbowArray |
|
#43 | interdiff-2476947-40-43.txt | 11.59 KB | RainbowArray |
#43 | 2476947-43-convert-page-title-block.patch | 26.59 KB | RainbowArray |
|
#40 | interdiff-2476947-37-40.txt | 26.18 KB | RainbowArray |
#40 | 2476947-40-convert-page-title-block.patch | 26 KB | RainbowArray |
|
#37 | interdiff-2476947-34-37.txt | 722 bytes | RainbowArray |
#37 | 2476947-37-convert-page-title-block.patch | 26.18 KB | RainbowArray |
|
#35 | interdiff-2476947-32-34.txt | 1.49 KB | RainbowArray |
#35 | 2476947-34-convert-page-title-block.patch | 26.01 KB | RainbowArray |
|
#32 | interdiff-2476947-31-32.txt | 1.61 KB | RainbowArray |
#32 | 2476947-32-convert-page-title-block.patch | 26.02 KB | RainbowArray |
|
#31 | 2476947-31-convert-page-title-block.patch | 25.9 KB | RainbowArray |
|
#28 | interdiff-2476947-26-28.txt | 5.64 KB | RainbowArray |
#28 | 2476947-28-convert-page-title-block.patch | 25.9 KB | RainbowArray |
|
#26 | interdiff-2476947-23-26.txt | 600 bytes | RainbowArray |
#26 | 2476947-26-convert-page-title-block.patch | 24.16 KB | RainbowArray |
|
#23 | 2476947-convert-page-title-block.patch | 23.57 KB | RainbowArray |
|
#15 | convert_title_page-2476947-15.patch | 23.59 KB | JeroenT |
|
#15 | interdiff-13-15.txt | 447 bytes | JeroenT |
#13 | 2476947-13.patch | 23.06 KB | andypost |
|
#13 | interdiff.txt | 1.69 KB | andypost |
#7 | 2476947-7.patch | 23.59 KB | rpayanm |
|
#3 | interdiff.txt | 5.49 KB | Wim Leers |
#3 | title_as_block-2476947-3.patch | 24.99 KB | Wim Leers |
|
| convert-title-to-block-1.patch | 22.1 KB | Manuel Garcia |
|
Comments
Comment #1
Wim LeersI'll take this on. Not assigning to me yet in case somebody gets to it before me. Please assign it to yourself so I know you're working on it.
Comment #3
Wim LeersHere's an initial bit of work. Fixes the bug where you end up with
<h1><div><h1>title…
, which obviously is not quite intended :) Also fixing some nits, and one test's failures.The biggest remaining thing is the title prefix/suffix stuff, which is very much broken in this patch.
Comment #5
Wim LeersNeeds to be rerolled after #2369987: Remove SafeMarkup::set() from 'head' title on template_preprocess_html landed. And of course, the 158 failures still need to be fixed.
Comment #6
Gábor Hojtsy#2369987: Remove SafeMarkup::set() from 'head' title on template_preprocess_html landed :)
Comment #7
rpayanmLet me try.
Comment #9
bill richardson CreditAttribution: bill richardson as a volunteer commentedPatch needs reroll
Comment #10
andypostComment #11
andypostnow we have 2
title_suffix
andtitle_prefix
, one in block and another in page.html.twig - suppose shortcuts needs other way to attachthis should be fixed,
Comment #12
Wim LeersIndeed, I said the same in #1:
Comment #13
andypostRe-roll and fix #11
Still need to decide for #12
Comment #15
JeroenTNot sure this is the right way, but this should fix a lot of the failing tests.
Patch attached.
Comment #16
JeroenT.
Comment #18
Wim Leers#15 That's unfortunately not the right way. It should be the right way, but it isn't, because we still allow
$page['#title']
to override the route's_title
or_title_callback
. The primary reason being Views, which has super-dynamic titles that may require lots of expensive computations to determine the title. We don't want to run those computations twice.(See #2403359: Use _title and _title_callback where possible and #2359901: Discourage $main_content['#title'] in favor of route titles and title callbacks for more about that.)
Comment #21
emma.mariaComment #22
RainbowArrayHere's a straight-up reroll of #15 so we have a starting point to fixing this. The local actions and tasks block conversion in #507488: Convert page elements (local tasks, actions) into blocks is getting close, so this is the last page variable conversion. Would be great to not have one oddball page variable in D8.
Comment #23
RainbowArrayAnd the actual patch.
Comment #25
Manuel Garcia CreditAttribution: Manuel Garcia at Appnovation commentedComment #26
RainbowArrayIt seems like some of the errors might be caused because the page title block wasn't in the install profile for Stark.
I'm not really sure what needs to be done with the title prefix and suffix, or what the shortcut title resolver should look like, but if I get this closer, maybe somebody else will be able to spot the quick fixes necessary to get this across the finish line.
Comment #28
RainbowArrayWell, the exciting thing is we probably have more errors and exceptions because the title block is actually showing up in Stark now.
This patch should take care of the doubling up of title_prefix and title_suffix. I've moved those inside the block in all cases, which seems much more straightforward. There was some weirdness going on with Seven's page.html.twig where the region was declared inside the h1 elements. Those have been moved into the blocks.
Maybe this will clear up some bugs?
It seems like the big issue is that SimplePageVariant and BlockPageVariant have arrays inside $this->title when those should be strings.
HtmlRenderer is one of the places that gets set, either from $main_content['title'] or titleResolver->getTitle, which does appear to return a string.
Ah.... I think I found it. BlockPageVariant can get the title from TitleBlockPluginInterface, and SystemPageTitleBlock sets title to... wait for it... an array. With the title inside #markup.
So maybe it just shouldn't do that? I'll try that and see what happens.
Comment #29
RainbowArrayComment #31
RainbowArrayHere's a reroll now that #507488: Convert page elements (local tasks, actions) into blocks is in.
Comment #32
RainbowArrayI feel like there's a decent chance I'm doing something wrong here. Trying to render the title #markup in order to avoid the array to string conversions. Guessing this is probably not the best way to handle this problem.
Comment #34
RainbowArrayTook some reading of the render docs, but I'm thinking this might help with the exceptions. Using renderPlain instead of render, since it should just be #markup in the $title, and there was a bug in how I was title #markup in BlockPage Variant.
I still expect there will be a lot of errors. Ultimately I think we'll need to enable block module and place the title block in many of the tests with errors, because that won't be placed by default. I'm hoping we can get the number of test errors down at least somewhat, though, because placing the title block up to 6,786 times seems... not appealing. There may be a cascade of errors from one missing title block, so hopefully it's a lot fewer times than that. Example code of how we can do this somewhat efficiently in #2562987: Enable blocks in testing profile and/or Classy.
Comment #35
RainbowArrayAnd the patch.
Comment #37
RainbowArrayThis should fix the preprocess exceptions. Hopefully moving in the right direction.
Comment #39
RainbowArrayPreviously, the page title was just in the page template, and we put h1 tags around in the system version of that template, and all was well.
However, by moving the title into the block template, we can still put the h1 tags around the title in that block's template, cool cool cool, but what about when blocks aren't enabled? In that case, rather than using BlockPageVariant, we're using SimplePageVariant. So the initial patch had used #markup in the build for that variant to insert the h1 tags. However... everything in core was expecting the page title to just be a simple string, not a render array. So... oodles of errors.
Just for kicks, moving the title back to a simple string for both SimplePageVariant and BlockPageVariant. That means there are no h1 tags for SimplePageVariant, so probably a new selection of test errors await.
The page template now no longer has a page title variable. It has a page.title region, right? So when we're setting page's title key in the build, I guess that can be reused elsewhere even if it doesn't show up in the template.
It seems pretty nasty to hardcode those h1 tags into SimplePageVariant now that I think about it. If for some reason somebody does have blocks all the way turned off and is just getting the SimplePageVariant, those hardcoded h1 tags would be super hard to override in the theme level. That seems really counter-productive for theming.
Comment #40
RainbowArrayAnd the patch.
Comment #42
ndf CreditAttribution: ndf commentedOne conceptual thing, imho we should not create a specific theme region for the title-block.
I guess it should be added to the 'Content' region, and probably rendered first/on-top by default, just before the 'Main page content'-block.
There are some related discussions about this regarding to menu's and message's:
In we must introduce a new region as needed to place the block, we could go for 'Before Content'.
Comment #43
RainbowArrayI think I'm finally beginning to understand, and the region tip was very helpful. Thanks, ndf.
The #markup for the title is creating a render array for the title, and when you look at how that is being done for messages, something similar is done so that messages show up even there is no placed messages block. However, that messages render array is placed inside the content, and I think it makes sense to do that here as well. I've set the weight of that array so that it appears below the messages.
I think that should clear things up considerably.
I also pulled out the title markup from the maintenance and install templates. One thing to note there is that Bartik had an id="page-title" on the h1 in its maintenance template. I'm not sure that it would be that straightforward to send a different block template for the page title just to Bartik's maintenance page. I don't see that ID being used in Bartik's CSS, though, so hopefully it's fine?
The highlighted region also sometimes appeared below the title, sometimes above. There are also two highlighted region placements in system page.html.twig for some reason. I'm pretty sure there's an issue to get rid of the highlighted region, though, so not worrying too much about that now.
Crossing my fingers and hoping I understand this now. I've definitely been wrong about that before.
Comment #44
RainbowArrayOh, also, I think the reason why there were so many failures before was at least in part because the $build placement of the title in a #markup render array was going into a title key on the page variants, but there wasn't a 'title' key: the region name was page_title. That's a moot point if we're not creating a title region anymore, but at least it explains why things may have been so broken.
Comment #45
emma.mariaThank you for removing the title region from Bartik in #43. I fully agree with #42. Bartik's page title is always printed within the content region. I will take a look at what is happening with the maintenance page when I do some manual testing.
Seven's page title however sits high up on the page above the header region, so it may well need a new region, but please do not call it Title. @lewisnyman can help provide direction on this.
Comment #46
RainbowArrayemma.maria: Yes, for Seven I placed the title block in the header region, which is that high up area.
Comment #48
RainbowArrayAt some point you'd think that Testbot would at least have the courtesy to say something amusing like "You know nothing, Jon Snow," each time I get things terribly wrong.
Trying to change the array key from 'title' to 'page_title' on the chance that 'title' does something special and magical, which seems entirely possible in the wonderful world of render arrays.
I traced back where $main_content['#title'] comes from for at least one of the exceptions, and I'm having a hard time where things are going wrong. Figured out that $main_content['title'] at least typically goes back to a controller setting a response, or sometimes the title is set right in a routing.yml if I am reading things right.
SimplePageVariant gets its $this->title (which is throwing the exception) from HtmlRenderer. The $get_title is getting the title either from $main_content['#title'] or titleResolver. I've traced titleResolver back before, and looking again, that either comes from running the route_title through $this->t (which is definitely a string) or from a title callback. So I suppose it's possible that a title callback somehow returns a non-string, and that's where the exception is coming from? In which case SimplePageVariant would be trying to concatenate the h1 tags with the title value, and if that's an array, I could see that being... exceptional.
So changing title to page_title is probably not going to solve this, but maybe it eliminates some weirdness somewhere.
At least I'm getting a hands-on tour of the render pipeline on this journey. Feeling foolish that I don't know enough to figure this out: if anybody has clues and suggestions, I'm all ears.
Comment #50
RainbowArrayLooking back to where the exceptions started. Fixing some small discrepancies in the install config file for Stark's page title block.
Comment #52
RainbowArrayChecked on simplytest.me, and the page title block is being placed in both Bartik and Seven. However the title isn't actually showing up on the page, perhaps not a surprise given all the errors.
I thought I had placed the Seven page title block in the header region, but apparently not. Fixing that now.
Also changing SystemPageTitleBlock back so that its title is a #markup render array. That is the thing that is not showing up on the page. Tracing that back, BlockPageVariant checks to see if a block implements TitleBlockPluginInterface, which this block does. If so, then it uses setTitle to send in the title.
The place where it gets that title is from HtmlRenderer, from that same $get_title() that SimplePageVariant gets its title from.
Ooh! So the Twig template actually gets the title variable from system_preprocess_block, and the page title block case gets that from ['content']['title']['#markup'], which I now realize is coming from the $build step in SystemPageTitleBlock.
This has been episode adventure of "Marc tries to understand Drupal 8." On the next episode, how many shades of yellow and red will Testbot turn after seeing what this patch does!
Comment #54
RainbowArrayI think what this boils down to is that in HtmlRenderer, $get_title() does not necessarily return a string, either from $main_content['#title'] or from titleResolver (possibly due to callbacks). For some reason, that can end up as an array.
On the other hand, these loads of exceptions didn't show up prior to me adding a page title block for Stark in the minimal profile. I can't think as to why that would cause a problem, but trying to remove that install to see what happens.
Comment #56
RainbowArrayWell that's interesting. So whatever is going wrong, it has nothing to do with the Stark page title block. That means we should probably base whatever the next patch is off of #52, not #54.
This makes me think that something changed between August 13 and August 31, when the first massive exceptions showed up. Obviously a lot of things changed in core during that time, but something that directly affected this code. Either something in the new code in these patches that isn't meeting new expectations or something went in that's clashing with these changes.
Comment #57
RainbowArrayLooked at change notices in that time period and git blames for some key files. Nothing new in SimplePageVariant, BlockPageVariant or TitleResolver. There are some new caching things in HtmlRenderer. It doesn't immediately look like that would mess up the way title is working, but it might be someplace to start looking.
Comment #58
RainbowArrayThe issue that affected HtmlRenderer was #2551989: Move replacing of placeholders from HtmlRenderer to HtmlResponseAttachmentsProcessor. It's possible other parts of the SmartCache work had an impact. In general SmartCache is setting up placeholders to be rendered later in the process so that non-cache context sensitive content can be cached, while more context sensitive things aren't cached along with everything else. Since the title is pretty dynamic and can be context sensitive, I'd imagine that would need to not be cached along with other things. So maybe title isn't being flattened as early leaving that an array rather than a string?
A lot of SafeMarkup things went in during this time period too. So that too could in theory have had an impact on how the title variable is handled.
Wim Leers would probably know what's going on with SmartCache, others probably know about SafeMarkup. If anybody has ideas, feel free to share.
Comment #59
Wim LeersWow, awesome work, @mdrummond, thank you so much for pushing this forward!
I found the root cause for your troubles. #2543340: Convert BlockViewBuilder to use #lazy_builder (but don't yet let context-aware blocks be placeholdered) made block rendering lazy, and special cased the main content block. The title block works in a very similar way (it carries state from the main request), and so we need to special case that one also.
I also found the root cause of the many exceptions: it's a bit tricky, but titles are plain strings 99% of the time, but they can be render arrays for the use cases where we need
<em>
in the title for example. So with a little bit of "if render array then use it directly, otherwise, map the string to a render array" logic, we can fix all that too.I didn't run all tests, but this should definitely help reduce the number of test failures.
This breaks whenever the title originates from the main content itself.
(See the hunks changed in
HtmlRenderer
.)This is why I think the Shortcut implementation must be rethought.
I see two options:
hook_block_view_alter()
, to alter the title block. Downside: Shortcut module will only work for pages built using Block module, so e.g. Panels (and other page variants) would have to implement a similar alteration on behalf of Shortcut module.#type => 'page_title'
, sohook_element_info_alter()
can be implemented by Shortcut module to make the necessary alterations.Hrm, I'm not sure this is still true. Especially with #2429617: Make D8 2x as fast: Dynamic Page Cache: context-dependent page caching (for *all* users!).
The title depends on the route (route name + params). Because it is the title for a specific route.
If we can cache the response for a route, which is the main content (== route controller result) plus the decoration (blocks) per route, then surely we can also cache a response's title for a route? They're deeply connected.
Fixed.
This is still implementing the old interface. Fixed.
Comment #61
RainbowArrayThis sounds like the better option to allow Panels, etc to not have to reinvent the wheel.
Thank you so, so, so much for all the fixes Wim!
I would bet that a good chunk of the remaining errors are from tests that need to be updated to 1) enable the block module, and 2) place the page title block in order to see the expected markup. I can definitely work on that, but I'm traveling today. I'll take a look tonight.
Comment #62
RainbowArrayAdding back the page title block for Stark in the minimal profile.
Comment #64
RainbowArrayHere's my attempt to update tests to place the page title block in places where it looks like that's necessary. It's entirely possible I'm wrong on that, but it's an attempt.
One thing is almost definitely wrong is the updates I made to the BlockPageVariantTest. However, I left a llama there in the hopes that Wim might be able to figure it out.
There was also an error on the test related to expected cache contexts. It said it didn't expect route, so I added that to the list of cache contexts that test was expecting. But it's entirely possible that is wrong.
I'm not sure how to fix the shortcut module.
I did check and titles are actually showing up again, which is definitely a step in the right direction!
Comment #66
Wim LeersI'm working on fixing the remaining test failures now. Making good progress.
Comment #67
Wim Leers:D You trick me!
This fixes all test failures that are unrelated to:
The solution for both of those is to have a
'#type' => 'page_title'
. Will do that in the next reroll.Comment #68
Wim LeersThis introduces
'#type' => 'page_title'
, and fixes the cache context-related failures.(Accidentally unassigned, now reassigning.)
Next: title prefix/suffix stuff, on which I might get stuck. Let's see.
Comment #70
Wim LeersI forgot to include one crucial file in #68… Therefore #68 will fail *very* hard :P
Comment #73
Wim LeersAh, great — as expected! The
route
cache context is now being added for every page title, and is thus causing quite a few test failures, because the expectations of many tests are outdated.Comment #74
Wim LeersSince I introduced
'#type' => 'page_title'
, empty titles (i.e. no titles) weren't handled correctly.Comment #75
Wim Leers'#type' => 'page_title'
's#pre_render
callback generates#markup
but theensureMarkupIsSafe()
call happens before that, hence we lose that safety guarantee. This took a while to figure out.This should be reviewed by Alex Pott.
Comment #76
Wim LeersConsiderations to solve
:title_suffix
can only be set in theme preprocess functions, not in render arrays (but perhaps I'm wrong!). So… that leaves only one choice: changeshortcut_preprocess_page()
toshortcut_preprocess_block__system_page_title_block()
. This means the handy way to create a shortcut (the shortcut shortcut I guess…) is only available when you use the page title block. Which could be acceptable.shortcut_preprocess_page_title()
andpage-title.html.twig
.For now, I went with option 2. But somehow, I'm getting the title suffix printed twice. I can't figure out why. Since I'm not even sure option 2 is the best choice, plus the fact that I don't know how this title-suffix-only-in-theme-layer thing is supposed to work exactly, I'm leaving that to you, @mdrummond.
I think the only other thing that still needs to be fixed is Views' contextual links. That's using all sorts of crazy hacks to work, and apparently we are breaking those.
Comment #81
Wim LeersI think changing
#markup
to#plain_text
would fixPageTitleTest
.And that makes sense too: the behavior in HEAD is that titles are escaped by default, unless they specify a render array with
#markup
and#allowed_tags
.We probably want to add an
assert()
call to ensure#title
indeed is either a plain string or a render array with those two keys.Comment #82
Frando CreditAttribution: Frando as a volunteer commentedIs it save to assume a page title only depends on the 'route' cache context? Couldn't I do something like this in a controller:
or even
Not sure, but wouldn't this break with smartcache then? I didn't test yet, but maybe the title block should inherit the main content's cacheability metadata.
Comment #83
Wim Leers#82: You're absolutely right. It will break even without SmartCache, just render caching of the block will already be problematic.
The solution is simple in principle: if you have such a dynamic title, you must not do:
but:
But this means that also all
_title_callback
s have to be evaluated, to check if they do dynamic stuff like this. Technically, this cacheability metadata should've been specified all along…Comment #84
RainbowArrayReroll of #76 due to changes in DisplayBlockTest.
Comment #85
RainbowArrayThis is an attempt to address the concerns with PageTitle, although I wasn't sure what to do with the assert request. Definitely need eyes on this to see if this is right.
My understanding is that for #markup there are certain tags that do not need to go into #allowed_tags, such as h1. So I'm not certain that #allowed_tags is required here.
In #76, option 3 sounds in theory good. Having a dedicated page title template is not out of line with some of the other block conversions we've done. I think in theory that might also allow us to skip putting the h1 tags in #markup, which would be good. SimplePageVariant might end up being used rarely for actual front-end work, but changing that h1 would be a pain if you needed to do so . So +1 to template.
Comment #90
lauriiiI was playing with this but there is a problem that in some use cases $element['#title'] has already rendered markup inside it which doesn't work well with the #plain_text :P I think we have to figure out where this markup is coming from
Comment #91
lauriiireroll
Comment #93
DuaelFrTake care, using TitleResolver can cause fatals on Views pages.
See #2516742: Allow Views to be resolved by TitleResolver
Comment #95
lauriiiThe title is already rendered in some cases when it comes to the PageTitle render element so closest we can get there is to use #markup. Why wouldn't we allow other types of render arrays than #markup? I removed that limitation and it seemed to fix the title at least for some of the pages :)
Comment #98
lauriiiReroll with a bit of a fuzz.
Comment #101
lauriiiI have some work in progress to fix this. Will be posting about my progress later today
Comment #103
tim.plunkettNot really in scope, this file is untouched otherwise.
Yet Another Special Cased Block That Contrib Cannot Mimic Because It Would Require Hacking Core
main content, messages, and title
Comment #104
Wim LeersNo, contrib can implement that interface too. So contrib can mimic it.
Comment #105
lauriiiWe might need to get #2359901: Discourage $main_content['#title'] in favor of route titles and title callbacks before this can properly work because otherwise we have to early render the main content which causes problems with the PageTitle render element. Other option is to keep extending the hack that we have there..
Comment #106
Wim LeersNo, we can't have #2359901: Discourage $main_content['#title'] in favor of route titles and title callbacks, it already seemed unlikely when I opened it, but this close before RC we can't do that anymore.
If HEAD can work with
$main_content['#title']
, then this patch should be able to as well.I can take care of that title escaping/already rendered weirdness, if you can handle the shortcut stuff. See #76.
The bigger threat to this issue IMHO is #82 + #83. We may want sign-off on that first. I'll bring it up during the D8 EU criticals call later today.
Comment #107
RainbowArrayThis creates a template for the page title which is used instead of the page title block template, which isn't necessary unless we want to remove the default block wrapper markup. Or at least this is my best guess as to how to do that. I may very well be wrong.
This then lets shortcut use preprocess_page_title. If this works, I like being able to always have a template for the page title even when there are no blocks. That is a lot cleaner.
Comment #108
RainbowArraytheme.inc probably needs to set template for page title render element like status messages does. Maybe. That's my last thought before slumberzzzzzzzz
Comment #111
dawehnerWe talked about that on the D8 critical meeting and came to the following conclusion:
Comment #112
RainbowArrayI think I was wrong about specifying the template for page_title. But this is still pretty broken. Definitely could use eyes from other theme team people on this.
Comment #113
joelpittetRemoved 'render element' as it's not needed and makes things unnecessarily trickier for preprocess.
And the twig {% extend @block %} left in the template wasn't needed and breaks things.
Comment #116
RainbowArrayI think this should help with any markup that might be in the title like em tags. Also fixing unnecessary prefixes and suffixes with h1 tags in a test.
I did manually check this, and page titles are showing up, which is good. I can add a shortcut from the title, so at least something is working there.
I'm wondering if maybe we should use a block template in order to remove the block wrapper markup around the title. That might not be necessary, as it looks like page titles look the same in Bartik and Seven. In Seven, the block contextual links go behind the main content area. That wasn't an issue before because there was no contextual links for the title in Seven. We might need a CSS fix for that.
Quick Edit sort of works. It appears that it's working, but after you save a change and reload the page, the change doesn't save.
Comment #118
cilefen CreditAttribution: cilefen commentedIn #116, the page title block is positioned below the page content on the freshly-installed home page with, the one with "No front page content has been created yet."
Comment #120
RainbowArray#118. Hmm. I'm not seeing that, at least not with Bartik.
Comment #121
RainbowArray#118: I'm not seeing that with a fresh install. We don't have an upgrade path yet, so if you're doing this with an existing site that might be why.
Comment #122
RainbowArrayWith #116, the span around title for QuickEdit is showing up in quotation marks. Because reasons.
Comment #123
Wim LeersTrying to get this to green today. Assigning to myself.
@joelpittet removed this in #113, and I thought "WTF".
@mdrummond added it back in #116 and I thought "yay".
But @joelpittet's change there was genius: this just makes it so that we bring back the exact same behavior as in HEAD. We have a title (either a string or a render array) that we pass to Twig, and let Twig handle it (and possibly render it). That's exactly what we were looking for in the last few comments/days! :)
So, bringing back that change, and removing even more that we then don't need anymore.
@joelpittet++
@joelpittet++
@joelpittet++
Comment #126
Wim LeersTo implement #111, which addresses #82 + #83, I see roughly four options:
#markup
or#plain_text
, and it needs to matchTwigExtension::escapeFilter()
's behavior, so that plain strings are escaped, and safe strings aren't.PageTitleSafeString
safe string implementation, which also carries cacheability metadata. But, this means that if the title is a plain string, we must already do the escaping that Twig would otherwise do, which means we can no longer access the unescaped title that we need for<title>
. So this does not actually work.#type => page_title
render array. This just requires some interfaces to be updated to not accept the page title as a string, but only as a#type => page_title
render array. Then we don't run into the "how to handle the different types of strings and mimic Twig" problem in option 1.\Drupal\Core\Block\TitleBlockPluginInterface
so it can receive the cacheability metadata of the main content block via a new method. This feels terrible. And still would only work for blocks; the solution must work for#type => page_title
, so that it also works for alternative page construction mechanisms (like Page Manager, Panels …).I implemented both options 1 (
solution-a-interdiff.txt
) and 2 (solution-b-interdiff.txt
).… and then after I'd done all this (few hours of) work, I realized there's a much more logical, and much more simple solution!
We're doing all this work so that the title block can be cached correctly. But, there is no value in caching this block! And caching the title block by all the cache contexts that the main content varies by, just means that the title block's cache redirect would eventually end up containing every possible cache context, which means that even the title for a simple route controller's main content, that doesn't vary by anything, still would need to vary by every possible cache context, i.e. the cache hit ratio would be horrendous.
That's a lot of complexity and overhead for actually even worse performance.
And the simple answer is: it does not make sense to cache the title block, just like it does not make sense to cache the main content block! Both are always calculated anyway by the route controller. So we should simply special case the title block just like we special case the main content block. That also makes it so that everything continues to work exactly like it does today, in terms of things being calculated/rendered. This patch then just allows you to move the title around thanks to it being a block.
Comment #129
Wim Leers#type => page_title
already is specifying theroute
cache context; the block doesn't need to do the sameRolesRidArgumentTest
had a very strange assertion, that I suspect was asserting a bug: it was asserting that if an entity has a label with<em>
, that that<em>
was output in escaped form. This patch fixes that. I expanded the assertion, to ensure that it is not just passing the HTML without any filtering.Comment #130
Wim LeersThe way the HTML's
<title>
tag is generated is very, very complex and forces us to duplicate a bit of logic inshortcut_preprocess_page_title()
. But the only alternative is refactoring the way that<title>
is computed, but that's very much out of scope for this issue; it's a problem that has existed for many years, layers of legacy over older layers of legacy. So I think this small bit of duplication is the better solution for now.This fixes the shortcut failures. If all is well, the only failures left now, should be the Views contextual links ones.
EDIT: Note that the second hunk here is a change made because
ShortcutLinksTest::testNoShortcutLink()
specifically asserts that the shortcut link appears even when there is no title.Comment #133
Wim LeersOops, I got the order/if-test of the first hunk in #130 wrong.
Comment #134
Wim LeersThere's far too much repetition in these templates, let's put Twig's power to good use to avoid that :)
Comment #137
davidhernandezI approve of extending, but there should definitely be a Classy version of the page-title template, and have Seven and Bartik extend off of that. I'm thinking it would go in the "content" folder.
Comment #140
Wim LeersSo, Classy would add
page-title
, since that's what both Bartik and Seven use? And then Bartik would extend Classy's, and just addtitle
?Comment #141
davidhernandezYeah, copy the system template to Classy and just extend Seven and Bartik off of that one. We have multiple reasons for having templates in Classy, not just if they are different from system. And if Bartik and Seven are going to extend something it would be from Classy.
Comment #144
davidhernandezThe interdiff looks a little funny because it thinks I moved the Seven template.
Also, this made me so happy.
Comment #145
dawehnerI've seen the use of #title => ['#markup'] so is this doumentation really the right one?
I'm curious whether this should call the old render() function which had that kind of logic
Does someone mind moving this to lib/Drupal/Core/Render/Block ... much like we did with the local tasks/actions blocks?
Is that a c&p? I don't see the option used
Should we document title_attributes as well?
Comment #147
Wim LeersHave to run. Did not run tests. But it does work in the UI. Perhaps the tests need to be adjusted.
All the INSANE work-around of Views plus contextual links is now GONE as a necessary requirement to get this patch to green, hurray! :)
Comment #152
davidhernandezpatch applied, not sure why it failed to run.
Comment #153
davidhernandezmanually testing 147, can confirm the contextual links are back.
Comment #155
Wim LeersFixed the fatal.
Comment #156
Wim LeersContextual links tests now pass.
Comment #161
davidhernandezOk, I think I can fix that now.
Comment #162
davidhernandezHopefully, this one's a winner.
Comment #163
RainbowArrayYAYYYYYYY!!!!
Now for last fixes!
#145
- 1. I don't know how to document something that can be either a string or a render array.
- 2. I don't know.
- 3. I moved this to Block as requested. For local actions we moved it to Menu, which was slightly different and perhaps a bit more logical place to look. But hopefully this still works? I like page_title_block a lot more than system_page_title_block as an ID. Maybe we should do a follow-up to move the other system page element blocks. Although I don't know what that upgrade path would look like.
- 4. I believe this is setting the block to not show the label for the block by default, which should be fine to do here. label_display is a default config option for all blocks.
- 5. Sure, documented title_attributes as well.
Next we'll need an upgrade path.
Comment #164
RainbowArrayThis adds an upgrade path for the page title.
One important note: I added a page title block to Classy for the standard profile. Right now there are no blocks enabled by default in the standard profile for Classy, but we should really have a follow-up to enable all the same blocks in Classy as are enabled in Stark for the standard profile. Classy is essentially what Stark used to be (with increasingly improved markup) , and enabling blocks for the standard profile shouldn't mess up tests, which as far as I know uses the testing profile in the cases where Classy is being tested by default.
I also added the page title block to Classy's content region in the upgrade path. That will help mitigate any issues with "Dude, where'd my page title go?" Although technically if a page title block isn't placed, it still shows up in the main content area, which works even better now that we have a page title template.
We still need to address 1 and 2 from #145. And we should get some good code reviews. But hopefully we are very close to RTBC and getting this in there. Thanks David and Wim for doing the heavy lifting of fixing all those tricksy tests!
Comment #165
RainbowArrayJust to double-check, re-uploading these patches as I had missed doing a commit before doing the patch and interdiff.
Comment #167
Wim LeersAddressed the remaining feedback:
Also fixed indentation nits I found in #163–#165.
Next: do a complete review of my own, and try to revert some of the changes along the way that turn out to be unnecessary in the end.
Comment #168
Wim LeersI believe #75 could be inverted completely. It's not necessary here, and hence out of scope. I moved that interdiff into a separate issue, because it's an independent problem: #2567715: #markup generated by #pre_render callbacks do not get processed by ensureMarkupIsSafe().
Slightly simpler patch!
Comment #169
Wim Leers#168 contained the same interdiff as #167 and was thus clearly wrong, here's the right one. (It's the inverse of #75.)
Comment #170
Wim LeersWe're placing the page title block in many tests now. But the whole point of the fallback is that … there is a fallback that works. So, reverting all of those.
Comment #171
Wim LeersAs of #137/#140/#141, the Classy template now has the
page-title
class on the page title's<h1>
. This means we can target the page title using that instead of the relatively hacky//main//h1
XPath expression that I used.Comment #172
Wim LeersBefore #126, the title block was being cached. So we need to ensure the right cacheability metadata was present. As part of that, we ensured that the
route
cache context was set on the title. This caused a whole avalanche of test changes.However, just like the main content originates from the route's controller, so does the title originate from the route controller, route's static title (
_title
) or dynamic title (_title_callback
). But we also don't set theroute
cache context on the main content. Arguably we should do that, but for now it's better for the title to be consistent with the main content. Because we now treat the route's title the same way as the route's main content, we should also treat their cacheability the same: neither are cached, and neither have hardcoded cache contexts set.This makes the patch a whole lot smaller. :)
Comment #173
Wim LeersThe only remaining thing I wanted to do is to make Views' contextual links work even without blocks — as of #147, a lot of the complexity in Views wrt contextual links for its Page display are gone, with only one negative side effect: Views' Page displays now only have contextual links if the "main content" block is being used.
Even though that only affects a minority of sites, I think that's the main thing left that people can complain about in this patch.
Together and yesterday combined I've now spent >6 hours on figuring out a solution to this problem. Sadly, there are many, many, many layers of rabbitholes with Views contextual links handling for Page displays. It actually dates back to the initial Views commit to Drupal 8 core (commit
a626abb24faa51ac140f73779a89e1ad7d5ae716
), which included the hacks necessary for Drupal 7 (grep forview_array
), and since then, the hacks have only grown more complex, mysterious and brittle (#1849822: Convert (HTML) view rendering to a render array, #2381277: Make Views use render caching and remove Views' own "output caching"). For example: we still haveview_array
, which itself was a hack around limitations in Drupal 7's theme system, added solely for the purpose of supporting contextual links, but then we don't ever populate it anymore in Drupal 8.Therefore, I propose we go forward with this patch without blocking it on resolving all of the crazy work-arounds for Views + Contextual links built up over the past >6 years.
Comment #174
Wim LeersHaving written #173, and having explored several approaches to fix it, I finally saw a solution! When using the fallback, the main content should get a theme wrapper that prints
title_suffix
, and that's all we really need!So, with this approach, you'll get contextual links for Page views even when not using the "main content" block.
Comes with expanded test coverage.
Comment #175
joelpittetMinor nit: extra space in there before attributes, not needed.
I'll do a bit of a thourough review later.
Comment #180
Wim Leers#175: fixed.
Fixed the two fails.
The
CommentPagerTest
was a WTF, especially because the test coverage added in #2346119: Fix call to undefined method Select::setCountQuery() did not make any sense. But that wasn't even the root cause, the root cause is that\Drupal\comment\Tests\CommentTestBase::commentExists()
is relying on regexps instead of actually traversing the DOM tree. The changes here trigger a false positive, and hence a test failure. No more regexp now.Comment #181
joelpittetThis seems like a great change but should it not assert the subject and body exist as it did before, do you think? Right now it's testing the structure but not the content, before it was doing both, although poorly.
You could check inside the match maybe for those two?
Comment #182
lauriiiYay!!
Unused
Is there specific reason why only render arrays with #markup are supported?
Might be out of scope but there might be use cases where someone could use a more general interface to opt-out block from being loaded lazily.
Nit: over 80 chars
Yay, cats!
Should be quick to add test also for a node page. Just in case they work a little bit differently.
Ubernit: Whitespace between break and new case :)
Hmm?
Should not be in the overriding template
I guess it might be same here
Comment #183
dawehnerDoes it really make sense to have a page_title render element? Maybe I just don't get it. I would be happy about an answer, maybe this helps me to understand the point of render elements better
Uh, we need a line explaining why we need this ... this is potential dangerous, given that it could hide issues on the update path. I know why, because Extension objects has the root stored, but I thought we fixed that already? We maybe need to apply the same fix to the ActiveTheme object
Seriously, this just makes it harder to read compared to just use "block.block.{$theme_name}_page_title"
The space is not removed yet
Here is an additional question: If we look at the current page template, the title is above the highlighted region, but here we set the block to the content region, not the highlighted reason, so are we sure this update is the right thing?
Comment #184
Wim Leers#183.1: It exists so that
shortcut
module and others have a way of adding prefixes and suffixes to the page title, because it can consistently target the page title, regardless of how it is rendered: in a block, via Panels …Comment #185
davidhernandezWhere do you see that? Title prints right above page.content.
Comment #186
dawehnerWell, right, they are using a preprocess function which is a theme level thing, isn't it, not a render level thing.
Ah I see, in the patch there are more special cases than normal cases.
Comment #187
davidhernandezI see, maintenance page ... sigh
Comment #188
dawehnerWell, some manual testing for those pages would be nice.
Did also anyone did some benchmarking for the case without dynamic page cache?
Comment #189
davidhernandezI checked the title in all the themes and the installer. This was all new installation. See attached files.
The only thing I found so far is that the contextual links to edit a node go a bit bonkers in Classy. I'm not sure why, since all the other themes are ok. Also, this only happens wit Basic page, NOT with Article. I wonder if it has to do with Basic having so few fields, and something coming out different in the markup. I didn't get a chance to look closely.
Here is a gif of what I saw: http://g.recordit.co/zGvJ7cg7G1.gif
I confirmed that the problem does not happen in head.
Comment #190
davidhernandezTo clarify, that flickering in the gif happens with the mouse curser still. I'm not moving it off the link and back on. The frame rate is rather slow in the gif.
Comment #191
davidhernandezWhen I apply the patch on a current install this is what I see:
No title in Bartik.
Seven title is at the bottom of the page.
Classy has title.
Stark has title.
Run update.php
Maintenance page has title.
Seven has title in the correct location.
Bartik now has title and in correct location.
And I can see that all four themes have the title block set in the block configuration.
Comment #192
davidhernandezDaniel was right to be cautious. We lose the title on the maintenance page. Is this because in maintenance mode blocks aren't active? Do we need to leave the variable printing there? Though that doesn't seem to be an issue with the installer.
Comment #193
davidhernandezI'm revising part of #180. I do see the link problem with Articles, and in Stark. But as soon as I add a tag to an article node it goes away. I'm assuming there is something about having just the body field with content. Maybe the area is too short or something odd.
Comment #194
RainbowArrayI just looked at the history of #2005546: Use branding block in place of page template branding variables (site name, slogan, site logo). We are not using the branding block in the maintenance and install pages, because blocks are not available there. So... we'll have to figure out how to make this work without the page title block on those pages here too.
Page titles should be showing up without blocks module being enabled. I don't know why that's not happening on the maintenance page, but it is on the install page. That's weird.
Comment #195
RainbowArrayThe page title is not displaying on the install page either. :/
Comment #196
RainbowArrayBefore
After
Comment #197
davidhernandezI didn't think to look at the installed. I meant the maintenance page when running the updates. See here:
Comment #198
davidhernandezIn my screenshot and what I'm seeing is that the big "Drupal" is the page title. The "configure site" is an H1 but not the title? What is it?
Comment #199
davidhernandezI see. I was looking at the wrong template. This is from Seven.
So the "Drupal" is just printing the site_name variable and the "Configure site" is title, but the page-title class is on the site name. Totally obvious. :/
Ok, so like maintenance mode the title isn't printing, because I think it can't make blocks at this stage.
I remember when we did the branding variables the variables had to be left so they could still work during the installation. I'm assuming this is the same case?
Comment #200
RainbowArrayTrying to address feedback.
#181: Seems like a good idea to check subject and body, but I'm not sure how to do that.
#182.2: Removed.
#182.3: We are no longer looking at #markup in the PageTitle render element, so I removed the #markup qualification from the comments here and in the RenderElement.#
#182.4: I don't know how to address that.
#182.5: Fixed.
#182.7: Added a test for the page title on a node page.
#182.8: Added space between breaks and new cases.
#182.9: I know Wim mentioned removing something with views_add_contextual_links, so maybe he can address that.
#182.10 & 11: Removed the @ingroup themable declarations from the Classy and Barik page-title templates.
#183.2: We used the same bit here in the branding block and local tasks and actions blocks updates. Not sure how to address these concerns.
#183.3: Updated. Again, that was used in the other block upgrade paths too.
#183.4: Hmm. I see that the space is gone.
#183.5: I'll address that in the next patch.
So areas where I could use help are:
- #181
- #182.4
- #182.9
- #183.2
Comment #201
RainbowArrayThe maintenance and install pages ended up being a relatively painless fix. ['page']['#title'] has the correct title value in template_preprocess_maintenance page, so I assigned that to $variables['title'] and then we can still use it in just in the maintenance and install templates, which can't use blocks. Those are weird and frustrating templates, but whatever. Not a big deal. We did the same thing with branding block.
This also doesn't mean we need to lose sleep over the page title being in different places in relation to the main content and status messages for maintenance and install. Now all is back to how it used to be.
Comment #204
RainbowArrayReverting the change I made for 183.3 since it caused a test failure. Maybe there's something I'm not understanding, but this version appeared to work.
Comment #205
dawehnerWe should better expand our test coverage to ensure that regression won't happen anymore in the future.
Its just ugly. Maybe alternative just concat the strings normally
Comment #206
Wim Leersviews_add_contextual_links()
is neither injected nor namespaced;\Drupal\Tests\views\Unit\Plugin\Block\ViewsBlockTest()
has the exact same work-around to let that unit test work.LocalActionsAndTasksConvertedIntoBlocksUpdateTest
andSiteBrandingConvertedIntoBlockUpdateTest
; so the same problem would need to be fixed there. Could we fix it in a separate issue, that happens in parallel to this one?Comment #207
dawehnerWell, fact is that doing anything before
runUpdates()
is problematic. Do you mind filing a follow up?Do we also have a follow up for the title on the install/maintenance page, even well, I'd kinda argue that its in scope of this issue to ensure that the title always appears.
Comment #208
lauriiiCreated follow-up for #182.4 and I think it can be worked later and be added in 8.1 etc. #2568035: Make it possible for blocks to opt-out from #lazy_builder callback
Comment #210
Wim LeersAnd now: profiling. Getting the flexibility of moving the title to any place by means of a block of course means some overhead, because in HEAD it is hardcoded. Moving from hardcoded to configurable always has a cost. So it's a matter of checking how big that cost is.
Profiling
This costs 1106 additional function calls, anon or auth, regardless of the page. Of course, only when Dynamic Page Cache is unable to cache a page.
Benchmarking
drush si; drush pmu -y page_cache; drush pmu -y dynamic_page_cache
ab -c 1 -n 1000 -g ab.tsv http://tres/contact/
Conclusion
A very small performance regression.
In my humble opinion, this is acceptable, since this is the very last block conversion, which will allow Drupal 8 to ship with a fully consistent page building model!
Comment #211
dawehnerWell, a 1% thing is something which sums up, just saying.
Comment #212
Wim LeersDrupalCI came back green. PIFR did not. For the UMPTEENTH TIME:
GET http://ec2-54-191-110-230.us-west-2.compute.amazonaws.com/checkout/user/login returned 0 (0 bytes).
. ARRRGHHHH! Kill it with fire!Re-testing…
#207:
Of course, done: #2568069: Block upgrade path tests refresh theme info before runUpdates().
Agreed. Done. This also revealed that the changes in #201 were in fact incorrect; the comment is correct, but the patch is not (and inconsistent). Fixed. The fail patch only contains the changes from the interdiff that do not affect Twig files.
#208: Thanks for filing that! Adding to related issues.
Comment #213
dawehnerComment #218
Wim Leers#2492839: Views replacement token bc layer allows for Twig template injection via arguments introduced that 1 fail here, probably because the changes in
RolesRidArgument
weren't fully kosher to begin with. That was the change I least understood, so it's great we no longer have to make any changes in that file :)Rebased and fixed.
Comment #219
lauriiiI've now reviewed the code few times and I don't see anything to complain anymore. I also think we can get over the 1% performance regression we have here.
Tested manually:
Title block appearing with upgrade path:
Title block appearing with clean install:
Also tried to remove title block from Bartik and title is still appearing on:
Comment #220
lauriiiWe could still use change record
Comment #221
Manuel Garcia CreditAttribution: Manuel Garcia at Appnovation commentedComment #222
davidhernandezAdded change record draft.
Anything in particular that needs updating in the issue summary? I deleted the todo. I didn't seem relevant any more and I deleting the line about adding a region.
Comment #223
davidhernandezI used an existing install, checked all the normal things. Applied patch and ran update.php. I can confirm the page title is now fixed on the update page, which uses the maintenance page template.
Confirmed the title looks fine in Seven, Bartik, Classy, and Stark.
Confirmed the title appears in maintenance mode for Stark, Classy, and Seven.
The title does not show up for Bartik. It looks like the hard-coded printing of the variable was put back in the other templates, but not Bartik's.
Also, I still see the contextual link regression in Classy. To reproduce, apply patch, update or don't, and try to use the contextual link on a node. It appears to happen to any of the nodes I try. It isn't all the links, just the one that appears for the node's body. And on the node's page (node/1,) not the front page.
It looks like this. I get to links, but clicking either doesn't do anything.
Can anyone else verify?
Comment #224
davidhernandezAlso, I just noticed what Lauri said about disabling the page title block. The title does still appear on the page. It gets printed below the content. Why would we want this functionality? This seems bad. I can't make the title go away and it automatically appears in a bad spot. Why can I disable it, if it is being made some kind of requirement?
Comment #225
RainbowArrayI tested on simplytest.me with this patch. Created a node. On the node page, I see the two contextual links: the top is for the block, the bottom is for the node and allows for quick edit. Both worked fine for me. When I hovered over one of the two icons and clicked, I got the dropbutton and was able to click on any of the links in the dropbutton.
This is in the current version of Chrome on my Mac with Bartik.
In Classy, I'm seeing the behavior described in #224. Can hover over the contextual links on the body, but it's fiddly, and clicking on the contextual links does nothing.
Previously, there was one contextual link when hovering over the body with three options: quick edit, edit and delete.
Now there are three contextual links: one when hovering over the title, which only gives a configure block option; and two when hovering over the body. The first body contextual link has configure block, edit (which edits the node) and delete. The second has Quick Edit (which edits the title and body of the node), edit (which edits the node) and delete.
In an ideal world, it would be nice if the contextual link for the title had Configure Block, Quick Edit (for just the title or title + body), edit (for the node), and delete, and if hovering over the body provided one contextual link that did the same thing. That may well be out of scope for this issue, but wanted to note this, which seems like a potential UX improvement.
Comment #226
RainbowArrayHere's a screenshot from Dev Tools for Classy on a node that shows where the three contextual links are coming from.
The page title block has one div with data-contextual-id inside it.
The article with the main content has a div.contextual-region around it with a div.data-contextual-id, and the article also has a div.data-contextual-id inside it: both of those have the same value for the data-contextual-id. I have to think that duplication is likely causing at least part of the problem here.
By contrast here is the markup in Classy prior to this patch:
So this makes me think the problem is more to do with any changes to the main content area than to the page title block. Maybe the title_suffix that was added to the main content?
Comment #227
RainbowArrayAs for the title appearing below the main content, that appears to happen in Bartik and Stark and Seven, but not Classy. In Classy the page title appears above the main content. I wonder what's different there?
We have a fallback in BlockPageVariant and SimplePageVariant to place the title in the content area if there is no title block placed anywhere. That's in part to help with tests where blocks cannot be enabled or to allow the title to appear if blocks aren't being used for whatever reason.
I would imagine something like Panels would have a different PageVariant where default title placement could be skipped if necessary? I'm not sure about that though.
Comment #228
RainbowArrayThis fixes the title on the maintenance page for Bartik.
I tried changing the page title weight on the render element placed if there's no block in BlockPageVariant and SimplePage Variant. Whether the weight is set at -900, -1000 or -1100, it still shows up below the main content, which seems really odd, especially because that does not happen in Classy. I don't see anything in the template or .theme for Classy that would make the page title work there but nowhere else.
I also tried removing the title_suffix from container--main-content. That did get rid of the double contextual link problem. If you place a main content block in Classy, there are still two context links in the body area, but they work.
But I thought there was a good reason for that title_suffix in container--main-content.
I'm not sure how to fix these issues, but maybe this gives Wim the info needed to come up with ideas for a solution.
Comment #229
dawehnerIn other words we need tests for that as well
Comment #230
Wim LeersThe root cause for #223/#226 is #174: the work-around I used to make a Page view's contextual links show up even when there is no main content block.
When there is a main content block, that automatically gets the contextual links from its contents. When there isn't, then in case of a Page view there is no template that prints
{{ title_suffix }}
. That's why I addedcontainer--main-content.html.twig
and set that as the default theme wrapper for main content (at the cost of an extra wrapping<div>
). But as a side effect of that, nodes (or any entity, really) gets their contextual links printed twice: once by the entity's template, and again by thecontainer--main-content.html.twig
theme wrapper.Looking at this again, I found yet another possible work-around. Quoting #173:
But the
views-view.html.twig
template does print a title suffix, it just relies on thisview_array
thing working correctly, so let's get that fixed instead. Then we can restore the old behavior for the main content block (no contextual links ever). I did keep the added test coverage that checks that the View has a contextual link both when the "main content" block is placed and when it is not. I merely adjusted the expectation (no "Configure block" link).Did all that, and now it's working just like in HEAD.
Comment #231
RainbowArrayThis adds a weight to the main content render array. The plus side is that if there are no blocks in the content region, the page title now appears above the main content. However, if there is a main content block but no page title block, the page title is still below.
Wim suggested turning off the #sorted flag on the $build for BlockPageVariant, which is a performance optimization. However, the sorting actually gets done in BlockRepositort->getVisibleBlocksPerRegion, with @uasort($assignment, 'Drupal\block\Entity\Block::sort'). So that isn't sorting any of the render arrays that might be placed in a region side by side with blocks. Frankly, I'm not sure how we go about doing that. But if we can, that will probably fix the order.
Comment #232
davidhernandezComment #235
Wim LeersI was able to figure out the root cause. I was right that it was the
#sorted
property inBlockPageVariant
. @mdrummond just was setting it on the wrong level :) He was setting at the region level instead of the block level inside the region that contains all the fallback page. So close! :)The reason that must be unset is because of:
… but when setting fallback main content/title/messages… that's no longer true! So when either of those are set, we must override that performance optimization again.
Furthermore, we must ensure
#weight
is actually set on the block render arrays so that we can sort them.Those are the two only actual changes I made. I just fixed the unit tests to make the expectations match the changes in #231. And I made the changes in #231 slightly more elegant.
So, all that is now left, is #229.
Comment #236
RainbowArrayComment #237
RainbowArray#236 adds a test for Bartik's maintenance page title, with a FAIL patch to demonstrate that the test works. At least hopefully it does.
In theory we could test any number of things with page titles across multiple themes, although I don't think that's something we typically do. That said, if adding this regression test helps, sure, no problem.
Comment #240
RainbowArrayWe now have a green patch that addresses all feedback both from code reviews and manual testing. I've updated the issue summary. There's a change notice drafted. Let's get this in!
Comment #241
lauriiiEverything seems to be addressed since my latest RTBC. I tested this again manually and I can see that maintenance page has now the title and also the title is on top of the page after disabling the block.
Comment #242
davidhernandezI'm still not sure I understand the title disabling thing? If you disable the block you still get a page title. Why can't disable the page title if I want to? This is a hard requirement? I'm thinking of things like not wanting the title on certain content types or the front page.
Comment #243
Wim LeersBecause we consider three things essential on a page:
Finally, both the main content and the title are things that are calculated for the current URL (route) you went to. They're both always calculated. Because they're both fundamental to the URL you're currently on.
Block module is core's layout system. If contrib adds additional ones, those can choose to not have fallback behavior, i.e. they can choose to NOT show the page title or the main content. But core needs to protect its users against shooting themselves in the foot. Without these 3 blocks, it's very easy to be either locked out (main content) or be completely lost how to make your site behave sanely again (title/messages).
Comment #244
davidhernandezBut that's not completely true. The node view doesn't have a title. You can create things without titles.
If someone wanted a page without a title, in the current system, there are straight forward ways to do that. If we are making the title a requirement of sorts, it is isn't consistant, and the UI is at least misleading. If you disable the block, it doesn't (to the unknowing eye) go away. If you alter the visibility settings on the block, they don't work. Limiting it to a specific content type, for example, controls the block visibility, but the default one then shows up at the top of content. To someone that doesn't understand the internals, this seems broken.
While all the normal settings are still there, the only thing you can actually do is move it around. That may actually be more limiting for a site builder than what we have now. At least someone could easily manipulate the title on the twig template, or even delete it. That isn't possible now. I think the most they could do is manipulate the contents of the page-title template, but that's hacky, and still leaves the block active.
Comment #245
Wim LeersAlright. That's pretty convincing :)
I'm personally not at all opposed to that. Let's do that then. Reverting only the necessary parts of #170's interdiff to bring back the "place block" calls in the necessary places. And removing the fallback logic. This should not fail too badly, and hopefully will actually be green.
Comment #248
RainbowArrayI went through the failing tests and placed the page title block where necessary.
Definitely agree people should have the ability to not have a page title with the UI. That does come up from time to time.
Comment #249
Wim LeersWoohoo! Thanks Marc :)
Nit:
{@inheritdoc}
on all thesesetUp()
functions being added. We have plenty of those in core already though, so I think it's fine to not fix this unless we need another reroll.Comment #250
RainbowArrayI wondered about that but saw a bunch of these without inheritdoc so left it off. Can certainly add that in if we need to.
Comment #253
RainbowArrayFixing remaining test failures.
Comment #254
RainbowArrayComment #255
Wim LeersAs far as I'm concerned, this is ready again. But I think it's up to lauriii or davidhernandez to re-RTBC :)
Comment #256
davidhernandezTesting now:
Installed using head. Checked lots of pages with titles; nodes, views, front, etc. Made screenshots to check the markup after applying the patch.
Apply patch. Titles gone. Running update.php Update 8008 runs.
Title there for the update. Title block appears correctly in the block layout list.
Titles appear correctly in Seven on admin pages. Shortcut works correctly.
Bartik:
Node titles are good.
Quick edit on node page is good.
View page title is good.
View quick edit is good.
User page title is good.
Disabling title works correctly.
Limiting the title by content type works.
Moving the title to another region works.
Title appears on maintenance page.
Classy:
The page title appears at the bottom of the page. I assume this is because there is no "main page content" block set. The default order puts the title below. o we need to fix this? I corrected this by adding the main page content block and ordering the blocks.
after...
Node titles are good.
Quick edit on node page is good.
View page title is good.
View quick edit is good.
User page title is good.
Disabling title works correctly.
Limiting the title by content type works.
Moving the title to another region works.
Title appears on maintenance page.
Stark:
Stark has the same problem as Classy. By default the title appears at the bottom.
Something is wrong with the blocks. When I try to edit the page title block it thinks it's the branding block. This is the path it links to,
/admin/structure/block/manage/stark_branding
which results in page not found. I can't disable the block either. If I add another page title instance, I getadmin/structure/block/manage/pagetitle_2
which is editable, but I can't do anything with the original.Stopping here with this part of the testing. Stark seems unusable.
Doing a fresh install with the patch applied.
Oh wait, on a fresh install all the blocks are correctly there in Stark. Is the upgrade path ruining an existing site's block setup? I seem to be able to manipulate the blocks just fine now.
Trying an install and update again. In head Stark's block configuration looks fine. Running update...
So now Stark's block config looks ok. :/ The problem I described above with Classy is still the case. I'm not sure if there is some fragility with the update. ? Can someone else try, and with content. The site I was using was brand new, and I didn't use it for anything else, so I don't know where there was an issue with the blocks in Stark. The only difference is the first site had content before updating, the second did not. I'll try again with content.
Ok, the first thing I did was install, and then turn on Stark. The block config had all the right blocks, but then Seven started misbehaving and not showing tabs. I reinstalled, turned on Stark, and now no blocks appear for Stark. What is going on? Can someone else confirm this oddity? Is there something wrong in head?
Comment #257
RainbowArrayI found a couple bugs that might explain some of the oddities.
On BlockPageVariant we should remove the weight added to main content: that's not needed anymore.
There was also a bug with Seven's install config for the page title.
I checked all the others, and they looked okay. I checked the update hook, and it looked okay.
Might be worth another try now.
Comment #260
RainbowArrayThis should fix BlockPageVariantTest. Tests now pass locally.
Comment #261
RainbowArrayManually tested on simplytest.me
- With Classy, title now appears above main content
- With Stark, title now appears above main content
- Editing blocks for Stark, this is the address I see for Stark's page title block: structure/block/manage/stark_page_title
- I am able to disable and enable the page title block in Stark
- I'm not seeing the oddity described with Seven and tabs
Since this is simplytest.me, I'm seeing a fresh install. I'll also try an upgrade locally.
- From 8.0.x HEAD, I did a fresh install.
- Then I switched to a branch with the latest patch applied.
- I cleared cache, then did an upgrade. I was advised of the page title upgrade.
- I installed Stark and set is as default.
- I checked all themes, and the page title block was correctly placed and seemed editable.
If you want to retest, David, maybe make sure to do a fresh install before you do the upgrade. Maybe you did that before, but that might help minimize any problems that could stem from testing other patches on your local. Maybe that wasn't what was going on, just a thought to eliminate variables.
Comment #262
mgiffordThat's pretty neat. I really like how you can just push the title off into the sidebar like that with this patch.
What needs to be done to mark this RTBC?
I did notice this issue with Seven's titles if you try to access the block, well you can't:
This could be fixed in a follow-up issue though.
Comment #263
RainbowArrayI checked with LewisNyman, and in general there are some problems with contextual links in Seven. He agreed that for this it made sense to file a follow-up rather than trying to deal with that here, since larger issues are involved.
Comment #264
Wim LeersYep, #262 is a pre-existing problem.
Comment #265
davidhernandezTesting now. Starting with a fresh install using head. (todo: check minimal install)
Before
Checking default block configs for all four themes, saving screenshots for later comparison, noting existing styles and positions to check for visual regressions.
Bartik: Site branding, Main navigation, User account menu, Status messages, Breadcrumbs, Tabs, Help, Primary admin actions, Main page content, Search, Tools, User login, Footer menu, Powered by Drupal
Classy: none.
Seven: Primary tabs, Secondary tabs, Breadcrumbs, Status messages, Help, Primary admin actions, Main page content, User login
Stark: Secondary tabs, Primary admin actions, Main page content, User login, Primary tabs, Status messages, Help, Breadcrumbs
Checking all themes in maintenance mode, making screenshots.
Patch applied
Titles gone (as expected, pre update script). Checking blocks. Bartik same, Classy same, Stark same.
...running update.php. Title exists in the same place, styles are the same. 8008 runs cleanly.
Checking post update block configs. Bartik same with page title at the top of content, Classy same with page title the only block in content, Seven same with page title at top of header, Stark same with page title at the top of content.
Quickedit
Bartik: Quick edit links work for the front page view and each node on the front page. Works on the body of each node's page.
Classy: All work the same.
Seven: All work the same with Seven as the frontend theme.
Stark: In Stark the page title quickedit is not accessible. The button is available and clicking it creates the little drop down, but it drops below the available area. This might be a Stark styling thing, but the other small blocks with contextual links, like tabs, work fine. I'll come back to this on a fresh install of head to see if it works the same.
Title styling
Bartik is the same, Classy is the same, Seven is the same, Stark looks same.
Maintenance mode
Bartik is same, Classy is same, Seven is same, Stark is same.
Block visibility
Bartik: Removing from front only works, limiting to Article only works, moving to another region works, removing completely works, putting it back works.
Classy: Works.
Seven: Works.
Stark: Page title block config does not work. This is the same problem I saw from before. It is overriding the branding block.
This is the problem.
edit: Those tags don't work, duh. The name and id are not the same.
Comment #266
davidhernandezIgnore the 8007. That code is part of hte 8008 update, but the patch shows it because the function is added after.
Comment #267
davidhernandezI'm checking install using head. The system branding block is not set in Stark on install, but it does appear in the update script. I'm assuming this is from the update when we converted the branding blocks. Is branding suppose to be part of Stark's default?
There is also something strange still going in head that I noticed the first time I tested this. On fresh installs using head when I enable Stark and set it as the default theme, the block layout can come out completely messed. Search, Footer menu, Tools, User login, Powered by Drupal, all end up in Left Sidebar. I haven't figure out what causes it. Sometimes when I install it happens, sometimes it doesn't. This is all in head, I drop the database and empty the files directory every time.
That obviously doesn't have anything to do with this issue, but I'm noticing it because well how many people are constantly installing and reinstalling and checking block layouts for Stark?
Comment #268
davidhernandezGoing back to the contextual link part of #265, all the contextual links I see work fine in Stark in head.
Comment #269
davidhernandezRe: contextual links problem in Stark, this works fine on a new install using the patch. I assume it is related to the problem of the upgrade confusing the title block with the branding block, so this problem may solve itself with that fix.
Comment #270
davidhernandezFrom what I can tell minimal install looks fine.
Comment #271
RainbowArrayFixed the Stark upgrade for page title. After doing this, the contextual link for Stark's page title appears to work. Quick Edit worked for me too with Stark page title.
Comment #272
RainbowArrayFor what it's worth, what #262 pointed out, that contextual links on Seven's page title aren't getting cut off, is caused because .content-header has overflow:hidden set on it. Remove that, and it works fine. This was never an issue before because in theory that region didn't have a contextual link that could overflow the region. And if there are tabs in that region, then there is enough vertical space that the contextual links won't overflow.
We'd need LewisNyman's feedback on that, though, as I'm not sure why that overflow hidden line is there. There might be a good reason for it.
Comment #274
LewisNyman CreditAttribution: LewisNyman at Wunder commentedOverflow hidden is required for tabs styling. It hides a border on desktop:
This is what happens on mobile when you disable it on mobile:
Hope this helps, I could spend some time trying to rework this implementation not to require overflow hidden but it is tricky because the mobile/desktop behaviour is tabs is so different, regressions are common.
Comment #275
davidhernandezThe contextual links issue in Seven is out of scope. It isn't created by this so we don't have to solve it here. There is also an issue to remove them completely in Seven.
Comment #276
davidhernandezThe fix in #271 fixes the problem I found with the Stark page title block and the Stark contextual link problem.
The Stark default block layout setup problem is completely bananas, but I'm having the problem with head, not this patch. So if we have a green patch I think it's finally good to go.
Comment #278
davidhernandezThat fails for me locally, so that's legit. Should the page_title_block placement be moved to the AggregatorTestBase setup()?
Comment #279
RainbowArrayTested this fix locally, and it appears to fix the test. Looks like a new test was added that checked the title, so we just needed to place the title block for the test. No worries.
If this passes, maybe back to RTBC?
Comment #280
RainbowArrayComment #281
davidhernandezI tested the change in #271 previously, and it was good to me. #279 fixes the test.
Comment #282
star-szrI'm partway through reviewing this, I may post a nitfixes only patch because that's all I'm finding so far.
One thing that may be disputable, so posting now:
I'm curious why this says "a #markup render array". Can't it just be any kind of render array?
In other words: Can we just copy and paste the comment from the implementation?
Comment #283
star-szrDoes this need an issue created and linked for the todo? Or does this need to be handled here?
The rest I found is all minor/nitpicks, see below and/or attached interdiff.
Added a trailing comma here.
Made these consistent (added blank line to AddFeedTest::setUp(), this patch adds more ones like the second one and core in general seems to like adding a blank line after the parent method call).
Added trailing comma here.
Changed all instances of block7 in this file to block6, this seems to go back to #64 but I don't see anything that means we need this gap in the sequence :)
Comment #284
davidhernandezThe todo was copied with the code from the ViewsBlockTest. So it doesn't need to be handled here.
Comment #285
RainbowArrayPatch no longer applied. Here's a quick reroll. Thanks for the review and fixes, Cottser!
Comment #286
RainbowArrayAlso, thanks for making the fix for #282. Agree with the change: does not need to be a #markup render array.
Comment #287
davidhernandezAdded follow up for the todo. #2571679: Replace views_add_contextual_links() in views.module with service
That link needs to be added to the comments in ViewPageControllerTest and ViewBlockTest.
Comment #288
davidhernandezAdding links for the todos.
Comment #290
davidhernandezI think we are back to ready.
Comment #291
catchAdding RC deadline tag, at least for now that seems like the right cut-off point, although also, last one! Not able to do a full review atm.
Comment #293
RainbowArray#2458763: Remove the ability to configure a block's cache max-age went in which removed the necessity and ability to set max age cache settings on blocks, which is likely what caused all the errors.
This fixes that.
There are still max age settings on the local action and tasks block upgrade paths. That should maybe be changed in a follow-up?
Comment #294
davidhernandezComment #298
Manuel Garcia CreditAttribution: Manuel Garcia commentedCurse the Gods! >:-(
Comment #300
RainbowArrayFails are almost assuredly due to #2567183: Re-export all built-in configuration in core going in. Probably not too bad a fix. Need to redo config files.
Comment #301
RainbowArrayComment #302
Manuel Garcia CreditAttribution: Manuel Garcia commentedThanks @mdrummond for getting this back to green!!
Not very clear as to what changed here, but shouldn't the provider be core?
Comment #303
RainbowArrayYou would think, but if you do an actual config export that is what you get. Same is true for other similar blocks.
Comment #304
Manuel Garcia CreditAttribution: Manuel Garcia commentedOK then, back to RTBC
Comment #305
Wim LeersAnd that is guaranteed to be correct thanks to #2567183: Re-export all built-in configuration in core having introduced test coverage that verifies all config is accurate.
Fingers crossed this will land soon :)
Comment #307
mgiffordComment #308
RainbowArrayLooks like reroll is due to #2550941: Allow passing contexts to display variants (to enable Panels Everywhere to use static context and relationships) going in. Investigating.
Comment #309
RainbowArray#2569083: Allow passing cacheable metadata to display variants (to enable Panels Everywhere) is also probably involved in the need for the reroll.
Comment #310
RainbowArrayQuick little reroll. Conflicts in HtmlRenderer and TestDisplayVariant due to the issues mentioned above. Nothing that took much sorting out. Move along, move along.
Comment #311
jibranBack to RTBC.
Quotspection...
Fingers crossed this will land soon :)
Comment #312
Fabianx CreditAttribution: Fabianx as a volunteer commentedRTBC + 1
Comment #313
dsnopekRTBC +1
Comment #315
RainbowArrayStraightforward reroll. Didn't even need to resolve conflicts. So this should be insta-RTBC-able as soon as tests are green.
Comment #318
RainbowArrayDuplicate system update number. Surprised that didn't show up as a conflict during the reroll.
Comment #319
jibranBack to RTBC.
Quotspection...
Fingers crossed this will land soon :)
Comment #322
Wim Leers#2575605: Field delete form does not display configuration dependencies that will be updated/deleted added
FieldUIDeleteTest::fieldUIDeleteField()
, which is failing. This indicates we must also place the title block in that new test.Comment #325
dsnopekPassing now...
Comment #326
catchComment #329
Fabianx CreditAttribution: Fabianx as a volunteer commentedBack to RTBC
Comment #331
catchI just reviewed this, and nothing stuck out. Upgrade path I spent a bit more time reviewing and that looks great.
contextual_preprocess() - can be fixed on commit. Did that.
This was on webchick's list for before the beta, but I don't see comments on this issue. I was going to leave it open for a few more hours to give her time to look if she wanted to review first, but then I realised it's Wednesday and ideally we'd release the beta today or tomorrow.
So I think it makes sense to get this in sooner than later - any fallout we have a bit of time to deal with, and this is the very last non-block page variable.
Committed/pushed to 8.0.x, thanks!
Comment #332
davidhernandez*throws confetti*
Comment #333
dsnopekHuzzah! Thanks, everyone! :-)
Comment #334
webchickTotally fine not to hold it up on me. :) I was just in contact with the themers and wanted to raise this as a concern.
Comment #335
Jeff Burnz CreditAttribution: Jeff Burnz commentedI'm so happy I'm crying tears of joy to see this in, just amazing and wonderful, so happy :)
Comment #336
Anonymous (not verified) CreditAttribution: Anonymous commentedWhere does the wrapping div with "field field--name-name field--type-string field--label-hidden field__item" classes coming from? I just cannot find the source of this damn thing.
---
The
<div class="field field--name-name field--type-string field--label-hidden field__item">Some title</div>
string is already present in the preprocess hook!!Comment #337
dawehnerInstall beta 16, it was there always. See
\Drupal\Core\Entity\Controller\EntityViewController::buildTitle
Comment #338
Anonymous (not verified) CreditAttribution: Anonymous commentedI jumped from beta 15 to RC1 so this is new to me. Anyway, how do I get rid of it? As I have mentioned in the preprocess hooks it is already provided as string so the only way is to use regex which is stupid.
---
Behavior of this thing is very weird and inconsistent. On taxonomy term page I am getting the aforementioned div, on my custom entity page I am getting nothing(ie proper
label
), on node page I am getting span(
label
).
This does not make any sense from developer nor themer point of view. How can I standardize what is being outputted??
Comment #339
Wim Leers#338: this issue introduced
#type => 'page_title'
andpage-title.html.twig
. You can implementhook_preprocess_page_title()
.I think that's what you're getting at.
Comment #340
davidhernandezThis should all be coming from the template. It sounds like you are using Classy, and that wrapper and class names come from the field--node--title template. That template wasn't introduced here, so you should have been seeing that wrapper on a node title for some time.
Comment #341
rootworkI think Wim has it right -- it's the new
page-title.html.twig
template. Took me a minute to figure that out too -- though having Twig debugging on helped, as it showed me right where it was!I've been loving this new implementation of the page title -- haven't run into the issues in #338 but maybe it all stems from that new template.
Comment #342
Anonymous (not verified) CreditAttribution: Anonymous commentedNo, the template nor preprocess function has no say in this. The title is already pre-rendered html string since it is rendered in \Drupal\Core\Entity\Controller\EntityViewController::buildTitle
Comment #343
davidhernandezYou asked where does the wrapping span tag and field level CSS classes come from. That is in the field template. The generic H1 on pages where the page title is not a field comes from the page-title template. buildTitle calls the renderer but it does not control the markup or classes.
Comment #345
cilefen CreditAttribution: cilefen commented#2605046: Contextual links on Page title block are confusing
Comment #346
cilefen CreditAttribution: cilefen commented#2353867: [META] Expose Title and other base fields in Manage Display