Toolbar can no longer switch horizontal and vertical -- expects breakpoints ordered from smallest to largest; responsive images need largest to smallest

Thank you for resolving the issue @mdrummond.

IMHO, ordering them in code would be better. You're right in saying it wouldn't make much sense to make a responsive image style off of those breakpoints, but i'd do it just for consistency.

@Jelle_S feel free to provide a better patch if you are so inclined, I'm just trying to resolve the regression. And this patch was MVC to do that.

I prefer the first one, it was explicit in the order defined by the weights in the yml. This obscures the expected order as it was defined and could even lead to confusion of how the weights work.

@mdrummond: if you just array_reverse() someone changes the yml weights and boom broken again.
If you want to force a direction, sort the array.

Still those weights are disingenuous if you start hardcoding the order overrides further down the line.

I like #12.
I think it's safe to assume that a module or theme maintainer knows what (s)he is doing when changing weights of breakpoints. If they don't, they'll probably look for examples in Drupal core. If we then use different logic in different places for assigning weights, that will lead to confusion. So for me adding the $reverse_sort parameter to getBreakpointsByGroup sounds good!

I have that theoretical layout system already in operation which uses Breakpoints. The recent patch to responsive images/breakpints reversed breakpoints and broke layouts that use cascading media queries, however I rolled with that because that was such a huge patch and I did not want to delay it anymore, but this is less than ideal I think because it breaks the mental model of how breakpoints typically work (small to large).

Personally I would have preferred it to be Responsive Images that ask for a reversed array and everyone else gets something that far better matches themers typical mental model, one based around how cascading media queries are typically used most of the time.

Thats just my 2 cents anyway, I can reverse the array and was going to do it today when I saw this issue pop up, if you end up leaving Breakpoints how it is and provide a reversed array for toolbar and my project, great, I'd be happy with that also, I'm not hard out arguing for major change, but just mindful of we are breaking the mental model here.

Good point Jeff Burnz. I'd prefer if the breakpoints are set back to the way they were and reverse them for responsive images as the exception.

It sounds like from mdrummond and Jeff burns that would be fitting the mental model? Please correct me if I've misinterpreted.

#15: That's the way I understood it as well. Sounds good to me :-)

@mdrummond That would be in these two places:

ResponsiveImageStyleForm.php line 103
responsive_image.module line 188

1. +++ b/core/modules/breakpoint/src/BreakpointManager.php
   @@ -176,6 +176,10 @@ public function getBreakpointsByGroup($group) {
   +          $breakpoints_reversed = array_reverse($breakpoints);
   +          $breakpoints = $breakpoints_reversed;
   

   I guess we can do this in one line.

2. +++ b/core/modules/breakpoint/src/BreakpointManagerInterface.php
   @@ -18,10 +18,14 @@
   +   *   The order for breakpoints, either 'small-to-large' or 'large-to-small'.
   

   not really sure about the possible values, it doesn't feel like Drupal, but it works for me.

   I think we mostly try to use a boolean, maybe something like $reversed_order = FALSE

1. +++ b/core/modules/breakpoint/src/BreakpointManager.php
   @@ -163,7 +163,7 @@ protected function findDefinitions() {
   +  public function getBreakpointsByGroup($group, $size_order = 'small-to-large') {
   

   I agree with @attiks likely should be a boolean.

2. +++ b/core/modules/breakpoint/src/BreakpointManager.php
   @@ -176,6 +176,10 @@ public function getBreakpointsByGroup($group) {
   +          $breakpoints_reversed = array_reverse($breakpoints);
   

   array_reverse() doesn't enforce that it will always be small to large or large to small, it just enforces the opposite of what was defined in the breakpoints.

   It's sorted be weight, so there is a lot of assumptions going on here...

#21 We need to use the weight, there's no easy way to sort breakpoints from "small" to "large", we had a look at it last year, but there's no manageable solution. That's why the weight was added, so a themer can order them.

Sort is done by: uasort($breakpoints, array('Drupal\Component\Utility\SortArray', 'sortByWeightElement'));, right before the array_reverse.

Not really sure, but do we need a (unit) test, to test the reverse_order

Couple of nitpicks:

1. +++ b/core/modules/breakpoint/src/BreakpointManagerInterface.php
   @@ -18,10 +18,14 @@
   +   *   If set to true, reverses the order of breakpoints as set by weight, to
   

   s/true/TRUE/

   And I think we generally use reverse (vs reverses) in core, but I'm not sure about that.

2. +++ b/core/modules/breakpoint/src/BreakpointManagerInterface.php
   @@ -18,10 +18,14 @@
   +  public function getBreakpointsByGroup($group, $size_order);
   

   s/size_order/reverse_order/

#2431465: the orientation toggle toolbar button is missing initially, until resize I think is a duplicate.

changes from #30 and...

1.
https://www.drupal.org/node/1354#types
boolean -> bool

https://www.drupal.org/node/1354#types

2.
https://www.drupal.org/node/1354#order
"Separate different-type sections by a blank line (for instance, all the @param documentation goes together, with a blank line before the first parameter and a blank line after the last parameter before the @return section starts). "

Assuming bot is happy, this should be fine.

I think committing this should be allowable, since the other patch broke the toolbar navigation, we probably need a follow up issue to add tests for the toolbar module.

Probably-duplicate: #2438715: Toolbar can no longer swap between horizontal and vertical mode on initial page load

Attempted reroll. One merge conflict:

diff --cc core/themes/bartik/bartik.breakpoints.yml
index cf28bed,17b9fc8..0000000
--- a/core/themes/bartik/bartik.breakpoints.yml
+++ b/core/themes/bartik/bartik.breakpoints.yml
@@@ -1,7 -1,7 +1,12 @@@
  bartik.mobile:
    label: mobile
++<<<<<<< HEAD
 +  mediaQuery: ''
 +  weight: 2
++=======
+   mediaQuery: '(min-width: 0px)'
+   weight: 0
++>>>>>>> patch
    multipliers:
      - 1x

Confirmed #2438715: Toolbar can no longer swap between horizontal and vertical mode on initial page load is resolved by #37.

Is there any way we could test part of this? Not the doodad itself, obviously.

If there is a CR, should it be on the issue that introduced the regression as well?

Related regression: #2439483: Toolbar styling is broken on minimal

Reroll from RTBC'd patch applies cleanly and said "doodad" is showing up after a clean install via manual test but yeah, not sure how this gets tested otherwise.

The new patch name including "bartik" is a bit of a red herring. Seems like we're touching seven, breakpoints and other stuff. Might be confusing for the core committers but thats such a strange nitpick that I'm still going to re-RTBC this based on #33.

+++ b/core/modules/breakpoint/src/BreakpointManager.php
@@ -176,6 +176,9 @@ public function getBreakpointsByGroup($group) {
+        if ($reverse_order) {
+          $breakpoints = array_reverse($breakpoints);
+        }

+++ b/core/modules/breakpoint/src/Tests/BreakpointDiscoveryTest.php
@@ -180,6 +180,15 @@ public function testModuleBreakpoints() {
+    // Test if breakpoints are returned in reverse weight order when
+    // $reverse_order is set to TRUE in getBreakpointsByGroup.
+    uasort($expected_breakpoints, array('Drupal\Component\Utility\SortArray', 'sortByWeightElement'));
+    $expected_breakpoints = array_reverse($expected_breakpoints);
+    $breakpoints = \Drupal::service('breakpoint.manager')->getBreakpointsByGroup('breakpoint_module_test', TRUE);
+    foreach ($expected_breakpoints as $id => $expected_breakpoint) {
+      $this->assertEqual($expected_breakpoint, $breakpoints[$id]->getPluginDefinition());
+    }

This is has to be outside of the static and and permanent caching. The test does not test the order at all. In fact the call to getBreakpointsByGroup() here returns exactly the same order as the first call to the method in this test.

Working on this...

Addressed both points in #42

The test did not complete due to a fatal error.	Completion check	AggregatorCronTest.php	19	
Drupal\aggregator\Tests\AggregatorCronTest->testCron()	
copy(/var/lib/drupaltestbot/sites/default/files/checkout/sites/simpletest/441189/settings.php): failed to open stream: No such file or directorycopy('/var/lib/drupaltestbot/sites/default/files/checkout/sites/default/default.settings.php', '/var/lib/drupaltestbot/sites/default/files/checkout/sites/simpletest/441189/settings.php') Drupal\simpletest\WebTestBase->setUp() Drupal\aggregator\Tests\AggregatorTestBase->setUp() Drupal\simpletest\TestBase->run(Array) simpletest_script_run_one_test('8', 'Drupal\aggregator\Tests\AggregatorCronTest')

random fail...

reviewing it now

Patch #44 works for me.
The button to move the tool bar from horizontal to vertical position is present again at larger window size, works correctly, and is not displayed on smaller window size.

Reviewing it properly now

Patch is looking good and CR is good

Reviewing this.

Thanks for the manual testing and the change record.

1. +++ b/core/modules/breakpoint/src/BreakpointManager.php
   @@ -163,7 +163,7 @@ protected function findDefinitions() {
        if (!isset($this->breakpointsByGroup[$group])) {
          if ($cache = $this->cacheBackend->get($this->cacheKey . ':' . $group)) {
   
   @@ -180,8 +180,10 @@ public function getBreakpointsByGroup($group) {
   +    $breakpoint_group = $reverse_order ? array_reverse($this->breakpointsByGroup[$group]) : $this->breakpointsByGroup[$group];
        $instances = array();
   -    foreach ($this->breakpointsByGroup[$group] as $plugin_id => $definition) {
   +    foreach ($breakpoint_group as $plugin_id => $definition) {
   

   Always doing this after the breakpoints are loaded from cache makes sense. Thanks @vijaycs85.

   Is it also possible to add test coverage for the cache invalidation? Are there steps I could use in manual testing to confirm that there isn't a caching issue?

2. +++ b/core/modules/breakpoint/src/BreakpointManagerInterface.php
   @@ -17,11 +17,14 @@
   +   * @param bool $reverse_order
   +   *   If set to TRUE, reverse the order of breakpoints as set by weight, to
   +   *   order them by largest weight to smallest weight.
   

   At first I was hesitant about adding this new complexity to the API, but after reading the issue, it seems reasonable. However, it's not clear from this documentation why I would want to reverse the order of the breakpoints. Can we clarify that?

I also reviewed the change record. Overall it's nicely detailed. The two different sections are a little confusing because it first makes a declaration about how breakpoints should be ordered, but then goes on to talk about the parameter to reverse them or not. Similarly to in my second point above, could we simplify the change record a little bit to focus on the fact that there is (if I understand it right), maybe a majority usecase and minority usecase for the ordering, and what examples are? It doesn't need to specifically say that "responsive image needed this but toolbar did that so we made this change", but we can still use those two modules as examples of when we'd use one ordering or the other.

I'm glad to see this patch nearly ready; this bug is annoying for sure.

We still need to answer these questions from #56

1) "Is it also possible to add test coverage for the cache invalidation?"

[edit:]
maybe see \Drupal\toolbar\Tests\ToolbarAdminMenuTest::testMenuLinkUpdateSubtreesHashCacheClear for an example

2) "Are there steps I could use in manual testing to confirm that there isn't a caching issue?"

3) and we need to update the documentation to address

"However, it's not clear from this documentation why I would want to reverse the order of the breakpoints. Can we clarify that?"

To find out how to write a reroll, check out this page: https://www.drupal.org/patch/reroll

Thanks for the reroll instructions @laurii.

Attached a first attempt to reroll it

Sorry forgot to switch issue status

Thanks for the reroll! Removing the tag because no reroll needed anymore :) We still need tests for this patch. Here's a great instructions for that: https://www.drupal.org/contributor-tasks/write-tests

I've tested the UI on a minimal install.

After enabling toolbar and breakpoint module, the button to toggle between horizontal and vertical display is initially not displayed.

When I reduce the size of the browser window, the toolbar automatically is moved vertically, but no button is displayed.

After increasing the window size again the button is displayed and can be used to toggle.

When I move to a different page in the site, the button is once again not displayed.

I guess there is a test coverage already

I've tested the patch in #62 and it still applies fine and resolve the issue.

I might be wrong but I think that if a module or theme only needs breakpoints to interact with css media queries it would make sense to reverse the order of breakpoints. This ways breakpoints are consistent with how we generally define media queries in css (smaller to larger).

If a module or theme needs breakpoints to interact only with responsive images then it should not use the reverse option.

If it use breakpoints for css media queries and images then it should play with the reverse option to have the good order depending on the need.

Thanks @idflood let's get this in:)

Defining them from mobile first perspective is great +1. And if they are gotten in reverse when they need to be reversed that also makes sense.

My only gripe is we do far too many array_reverse calls in the theme system than should be necessary but that is unrelated to this case:P

Discussing with @mdrummond and @webchick on IRC. The new param needs better doxygen comments to explain when to use it. But I'd be inclined to just remove it and if someone needs the reverse they can reverse the output after they call it (responsive image I'm looking at you)

The changes to the weights in this patch look wonderful still though, I'd not change those.

Thank you for updating the comments to make them clearer, I think that helps a bunch.

And this approach seems a bit nicer than adding a new param. Back to RTBC.

Awesome, this is much more along the lines of what I expected. Thanks too for the greatly expanded comments in responsive_image module explaining why these backflips are necessary.

The only other nit I see is:

+    // Test if breakpoints are returned in reverse weight order when array
+    // returned from getBreakpointsByGroup is reversed, as needed by Responsive
+    // Image module.
+    uasort($expected_breakpoints, array('Drupal\Component\Utility\SortArray', 'sortByWeightElement'));
+    $expected_breakpoints = array_reverse($expected_breakpoints);
+    $breakpoints = array_reverse(\Drupal::service('breakpoint.manager')->getBreakpointsByGroup('breakpoint_module_test'));
+    $this->assertEqual(array_keys($expected_breakpoints), array_keys($breakpoints));

At this point, that test is literally just ensuring array_reverse() works, which is not our job. :) So removed that chunk and...

Committed and pushed to 8.0.x. Thanks! So great to have this one put away!