Support from Acquia helps fund testing for Drupal Acquia logo

Comments

tim.plunkett’s picture

Title: The Views wizard is broken after fieldsets were replaced with details » The Views wizard looks terrible after fieldsets were replaced with details
Issue tags: +VDC

This is bad for usability, accessibility, and aestheicsability.

sun’s picture

Priority: Major » Normal
Status: Active » Needs review
FileSize
14.69 KB
7.37 KB

This gets the basics done.

As mentioned in the original Details issue already, Views generally contains lots of markup and CSS it cannot and must not contain as a core module.

The benchmark is Stark, not Seven:

views.admin-theme.2.png

tim.plunkett’s picture

Issue tags: +Usability
FileSize
36.65 KB

Right, but we use Seven when considering usability.
And we just fixed up this page.

newaddview.png

sun’s picture

Sorry, but no, usability is seven-agnostic in core. At the very least, when it comes down to markup and CSS.

This UI has to work in other/custom themes, too. And it needs to work well. The only way to achieve that is to make it work in Stark.

But I can't see what's wrong with that screenshot of Seven in the first place? Looks sane and just about right to me?

sun’s picture

That said, this patch might be a good first step, but this is only ~1% of the problem space. Views' CSS violates core standards on many different fronts. Fixing that will be a very long and painful ride.

Ultimately, I'd rather recommend to delete the entirety of Views' CSS, and re-implement it from scratch, fixing the markup along the way. There's nothing to lose with the current markup/CSS, except of many problems.

I'm aware that this a radical suggestion, but I'm fairly confident that we won't find the front-end resources to bring Views' markup and CSS up to par with core's front-end and coding standards otherwise.

xjm’s picture

Er. Views' CSS was already cleaned up significantly for Drupal 8.

tim.plunkett’s picture

#7, that's correct, but I think Views UI CSS is being called into question here, which had much less work done on it.

However, there is no reason to put the Views UI into a non-functional state while a clean-up is hopefully finished.

sun’s picture

Yeah, sorta. Essentially, this very issue probably serves as a good benchmark already.

While I think that this patch is complete and fixes Views UI's Add screen, we now need someone who is willing to test the entire Views UI - in all core themes - to make sure that everything still works as expected (and additionally verifying that a potential bug is not a pre-existing bug).

That's a tedious job on its own. And regardless of how much you test it, someone will complain anyway.

This patch only kills a small fragment of the offending CSS in Views UI, which should not exist in the first place (because it affects all markup on a page, not only Views', and also, because every single of those overrides has to be undone by every single theme). Therefore, this entire process will have to be repeated for every single change to Views UI's CSS and markup. Which will take ages. And an army of front-end people who love to do full-stack QA (i.e., no one :P).

That's why I think that doing this in small steps is utopian.

Bojhan’s picture

Can we restore the look of this to what was in core before the details patch. The fieldsets where there to give visual structure to the page and to contain similar functionality that visual structure is now lost.

Cross post with @sun, the patch is not complete - because it does not instill the correct visual structure we are after.

sun’s picture

Regardless of this particular issue, in light of #9, I think I see two options only:

A) Skip the full-stack testing and instead, rather move forward quickly; i.e., commit patches like this as they arrive and are confirmed to solve a particular problem. If a commit causes problems elsewhere, file follow-up issues. In short: Pragmatism on steroids.

B) Ditch and delete the entire CSS and redo it from scratch. Have a single issue for doing that, and only perform full-stack QA on this single issue.

--

A) will take weeks, but at least UI changes + improvements happen steadily and nothing is left broken as we proceed.

B) will take months, won't fix intermediary problems in between, and at some point the entire thing will have to be committed, regardless of its current/final state.

sun’s picture

One additional thing that I recognized in Views UI's code and CSS:

There is a .fieldset-no-label class, which is bogus on all fronts. Eliminating that and replacing affected things with a simple and proper #type 'container' and probably a corresponding .views-container class (as it seems like the entire purpose of abusing fieldsets without legend was to get a border around a DIV container [Really? ;)]) will probably fix a dozen of different things in the UI already.

It might be worth to split that out into a dedicated issue.

Can we restore the look of this to what was in core before the details patch. The fieldsets where there to give visual structure to the page and to contain similar functionality that visual structure is now lost.

This would have to be done in the Seven theme then.

But off-hand, I do not understand why we suddenly want to special-case this particular UI screen versus the entire rest of Drupal's administration interface. Literally all other "Add [whatever]" forms throughout Drupal core look like #3 + #4, so I don't get why Views should look differently? What's the reasoning for that?

If there's a case to be made for a new visual/interface pattern for forms, then I think we should discuss that in a dedicated and isolated issue.

Bojhan’s picture

@sun Thanks for reconsidering. I don't think its reasonable to do the one mammoth issue approach, that will be incredibly hard to get contributors towards - but I think we could all applaud if you want to undertake that. I think it was only expect able as major core contributors go through Views, you would find big parts that need overhaul.

We do need to fix places in the Seven theme where we broke the visual hierarchy, the details patch sadly broke a lot more than I expected. There is also quite some places where we used field sets wrong, I am glad many of those are gone. I don't care so much about the semantics, all I want is my borders :) - but I am sure our screenreader users, also wouldn't mind capsulating of this functionality.

The views UI is very much a one-off, many of the patterns you won't find elsewhere in core. I'm not happy about that, but that is the situation we are in - I do not think making the situation worse, by removing the things that make the Views UI more usable, is a useful strategy.

webchick’s picture

Status: Needs review » Active

sun, Can you please stop using totally inflammatory words like "abusing"? It's really frustrating, and disparages the work of others.

The Views UI as it stood before the details patch went in has gone through countless usability studies showing that what was there in the wizard worked, both for new users and advanced. Presumably, a big reason why it worked is because those sections in the form were all labeled, thus gradually introducing people to the terminology that the advanced interface shows. The patch introduces a regression which removes those terminology cues, and turns the whole form into a single glob that's undifferentiated. Therefore, it's not a suitable fix, and I'm reverting this back to "active."

We need to simply restore what was there before the details patch went in, or else we need to roll back the details patch until this can all be sorted out. Changing the look of this form to remove the fieldsets, if we decide that we want to do that, and can do that without adversely harming usability, can be done in a separate issue.

sun’s picture

Status: Active » Needs review

Presumably, a big reason why it worked is because those sections in the form were all labeled, thus gradually introducing people to the terminology that the advanced interface shows.

The sections are, in fact, not labeled. If they were, then you would see code lines involving #title being removed in this patch. But there are no such lines.

The entire purpose of those fieldsets clearly seems to have been to achieve containers with borders. In other words, pure styling.

If the only purpose of a container is visual styling, then a DIV element is still the right choice, even in times of HTML5. The spec explicitly states that.


I do not perceive "The UI is special, because we made it special." as any kind of valid argument. It lacks every possible and sensible foundation. ;) If you make that an argument here, then I will also make it an argument for other, non-standard UI changes elsewhere in core. :) We can do that, but neither of us will be happy with the end result, and especially themers will hate us for doing so.

This patch brings the "Add view" form/screen up to par with current core standards. If you want to add special styling for that page to Seven theme, feel free to do so.

Please bear in mind that Seven theme is not used by everyone. Drupal core cannot assume that and must not assume that.

webchick’s picture

Ok, well I guess I'm rolling back the details patch then? We exchanged what's essentially a feature request for a major regression in one of our main core UIs. I'd rather not roll it back, since I think details is both semantically more correct and also has nice accessibility benefits. But that doesn't seem like a smart trade-off to me.

webchick’s picture

Also, I'm perfectly happy to entertain discussions about bringing Views front-end code more inline with the rest of core's but atm we have a straight-up regression. It needs to be fixed, or we need to undo the thing that broke it. It's really that simple.

sun’s picture

I worked an hour to prepare a longer comment that explains the overall situation on objective grounds, which I intended to post. However, the existing patch and the substance of my comments have been ignored. I therefore conclude that every perspective on the problem space that is different to the one being stated in earlier comments is willfully ignored. I do not see myself in a position to provide further information that will not be ignored.

webchick’s picture

Hm. I certainly don't mean to ignore your points, I'm just pointing out that they're off-topic to the issue at hand, which is: we need to restore the visual look of the Views UI wizard from before the details patch. That's all.

Everything else you've brought up are definitely points worth discussing (we of course want Views's CSS improved and brought inline with the rest of core's before release so we don't cause any themers to stab themselves in the face ;)), but not as part of resolving a major visual regression. Kittens and all that. :) This should be a 4-line patch or something that we can commit in 8 seconds, and then allow us to carry on the larger meta discussion of unifying Views styling with the rest of core.

Hope that makes sense.

webchick’s picture

Priority: Normal » Major
Status: Needs review » Active

Restoring properties.

Bojhan’s picture

Priority: Major » Normal
Status: Active » Needs review

@sun I don't feel like I ignored your comments, I believe the visual hierarchy that Views used to have was adding a lot of usability. That seems to be primarily what others are arguing for too.

More importantly, your fundamental concerns give the impression you don't want to fix this visual hierarchy bug - which would help a lot if you can clear that up, because if you disagree with this - then it is indeed better not to spend more time on this issue. You are sounding concerns around different points which are special styling and semantics. Although obviously we want to keep special styling to a minimum, we do want to use it when it adds significantly more usability. You can disagree with this, but this is the premise on which most of the Views UI is in core - a lot of it is special, perhaps we should agree to disagree on this :)

I don't care much about the semantics, and I don't think others in this issue are actually disagreeing with that - people just want to see the bug fixed. I think as often happens, we are miscommunicating focusing on completely different verticals. I also don't think we should really drag this out much longer, this is a silly tiny issue :) in the scheme of things.

Bojhan’s picture

Priority: Normal » Major
Status: Needs review » Active

meh, webchick always types faster :D

sun’s picture

Hope that makes sense.

Unfortunately it does not. Instead, it further tries to manifest a truth that I'm not willing to accept:

If I get the sense of the comments of this issue right, then we're applying double-standards to Drupal core now. The second set of standards seems to apply to Views only. That second set of standards is neither explained nor documented in any way. Not in the same way our primary standards are documented and known to everyone contributing to core.

This is further underlined by the fact that it appears to be impossible to contribute changes to Drupal core right now, in case they happen to touch or affect Views. Proven by this issue.

Due to earlier comments, I'm no longer allowed to speak about the state of Views. An unbiased and objective in-depth analysis of its current state will reveal most of what anyone could reasonably say when evaluating it against our (primary) core standards.

webchick’s picture

Hm. No. The way to read the comments is "We committed a patch that caused a visual regression. We need to undo the damage, and quickly, because this is a major bug, and holding up features. Further refinements to the Views UI, markup, and surrounding CSS are welcome in follow-up issues after this basic dumb regression is fixed, so that we have ample room to discuss them properly." I'm not sure how I can state it any plainer than that.

sun’s picture

No.

Good to hear. If we do not apply double-standards to Views, then the existing patch fixes Views UI in the exact same way any other UI in core would be fixed, if it would show this problem. By definition, there cannot be a visual regression, because the UI did not follow our existing core standards in the first place.

In other words: There's no way to resolve this issue without choosing one of the following options:

Either we're applying double-standards, or we are not.

webchick’s picture

Status: Active » Needs review
FileSize
52.61 KB
2.73 KB

Heh, now who's ignoring whom, eh? :)

This looks like what we need, give or take. We should also open a major follow-up task to resolve some of the general consistency concerns sun's brought up above.

No more 'Details' drop-downs on fieldsets on the views wizard

sun’s picture

Status: Needs review » Needs work

That fieldset styling will break as soon as the obsolete fieldset styles are removed. The accessibility team wants to use fieldsets as fieldsets.

By changing the #type from details to fieldset, the underlying problem only gets deferred to another issue, but is not resolved in any way.

sun’s picture

Title: The Views wizard looks terrible after fieldsets were replaced with details » Update Views wizard for details
Status: Needs work » Needs review

In essence, #3 is still the right fix and proper patch.

If it violates any core standards regarding code, CSS, markup, forms, usability, or theming, you could have pointed me to them, and I would have fixed them accordingly.

That did not happen. Instead, it looks like Views is a custom silo in core that follows its own, undocumented standards. In that case, no Drupal core contributor can reasonably help to fix related issues. If that is what we want to pursue as a community, then we surely can - although I'd gently ask for some documentation for how to deal with these kind of situations, since they can very well hold off patches that are perfectly fine otherwise. What we're essentially facing here is the implementation and maintenance of a very custom design in core, and the design guidelines are undocumented.

Since I introduced the "regression", I took it as my own responsibility to correct (un)intended consequences of the original details patch. Therefore, I provided a patch to correct the situation in #3. This, however, does not seem to be possible, since the patch follows Drupal core standards, but those do not seem to apply here. In turn, I do not see myself in a position to fix the user interface problem that has been revealed by my patch.

In turn, I also find it very offending to complain about this as a "regression" in the first place, because it is only natural to expect all kind of regressions with regard to any kind of custom design/implementation; especially if other contributors are factually not able to update/change/correct it. Therefore, adjusting the issue title.

The patch in #26 is obviously fine, but as mentioned before, this will break as soon as we're removing the all the custom CSS styles for fieldsets. The accessibility team desperately wants to move forward on fieldsets, so when committing this patch, I at least hope that you will not block their efforts on the fact that Views UI still **uses fieldsets — the entire purpose of removing their previous usage was to allow to use fieldsets as... fieldsets. Thanks.

chx’s picture

Status: Needs review » Reviewed & tested by the community

In line with #24, I am calling this done.

To answer sun's concern whether there are double standards: Drupal 8 is a strange animal. Our previous processes do not hold up for the sweeping changes we wanted. We commit patches incomplete, broken and slow in the hope that by the time we release they will get smoothed out. This is not Views specific. It is frustrating, teeth gnashing, I-want-to-break-something frustrating, incredibly hard but together we can get there.

chx’s picture

Title: Update Views wizard for details » Add Views wizard clues are lost
tim.plunkett’s picture

I can confirm that the patch in #26 restores the Views Wizard to the visual state from before the fieldset/details patch.

I also realize that the Views team will need to be proactive in adopting some of the newer approaches/decisions made concerning the proper usage of render/FAPI elements, since we do use them in extensive and creative ways.

webchick’s picture

Status: Reviewed & tested by the community » Fixed

Ok, committed/pushed #26 to 8.x for now, so we can get rid of the obvious visual bug. Opened #1855120: [meta] Clean-up Views UI CSS/markup to bring it more in-line with core's as a place to strategize about the best approach for bringing Views's markup/CSS more in-line with core's. Will fill out the issue summary shortly.

Automatically closed -- issue fixed for 2 weeks with no activity.