WordPress.org

Make WordPress Core

Opened 3 years ago

Closed 10 months ago

#42001 closed defect (bug) (fixed)

Active Widget Not Fully Visible

Reported by: fervillz Owned by: audrasjb
Milestone: 5.4 Priority: normal
Severity: normal Version:
Component: Widgets Keywords: has-patch has-screenshots commit
Focuses: ui Cc:

Description

Patch to add "active" class to selected widget and have it fully visible. When you click a toggled/open widget, the widget should be on the front of the other widgets.

http://sheferstudio.com/wp-content/uploads/2017/09/wp-issue-active-widget.png

Attachments (14)

wp issue active widget.png (322.4 KB) - added by fervillz 3 years ago.
42001-v1.patch (916 bytes) - added by rinkuyadav999 3 years ago.
42001.patch (915 bytes) - added by jaydeep23290 3 years ago.
UI Issue.png (18.7 KB) - added by ashokrd2013 3 years ago.
Widget Issue.png (15.9 KB) - added by ashokrd2013 3 years ago.
42001.1.patch (1.1 KB) - added by ashokrd2013 3 years ago.
52dd9bb87ab970d65999cb8759341632.gif (403.9 KB) - added by audrasjb 20 months ago.
BEFORE widget text
14edcd8d766f2683409069feb243fdb6.gif (379.8 KB) - added by audrasjb 20 months ago.
AFTER widget text
2019-06-20_1200[1].png (114.0 KB) - added by ov1diu 18 months ago.
Active Widget Not Fully Visible Ticket ID#42001
42001.2.diff (1.6 KB) - added by audrasjb 17 months ago.
Reducing widgets width to 250
083bbff4d2b6fc341b4f85e94eb07622.gif (3.3 MB) - added by audrasjb 17 months ago.
Fixed widget width for all wide widgets
42001.3.diff (977 bytes) - added by SergeyBiryukov 10 months ago.
42001.4.diff (1.2 KB) - added by SergeyBiryukov 10 months ago.
07e137f9557a71426dbadb5e468e1e78.gif (963.0 KB) - added by audrasjb 10 months ago.
Works fine!

Change History (50)

#1 @fervillz
3 years ago

  • Severity changed from normal to major

#2 @westonruter
3 years ago

  • Focuses javascript added
  • Keywords needs-patch good-first-bug added
  • Severity changed from major to normal
  • Version 4.8.2 deleted

#3 @rinkuyadav999
3 years ago

  • Keywords has-patch needs-testing added; needs-patch removed

@jaydeep23290
3 years ago

@ashokrd2013
3 years ago

#4 @ashokrd2013
3 years ago

  • Keywords needs-patch added; has-patch removed

Below 2 files have parameter set for height & width, which needs to set 250 for height and width. It will display these 2 widget like other default widgets (Gallery, Tag etc..) and fix the issue related to design.

If size not made 250 then another issue also occurs see below screenshots.


src\wp-includes\widgets\class-wp-widget-custom-html.php
src\wp-includes\widgets\class-wp-widget-text.php

Below is the fix for all issues.


@ashokrd2013
3 years ago

#5 @ashokrd2013
3 years ago

  • Keywords has-patch added; needs-patch removed

#6 @DrewAPicture
3 years ago

  • Owner set to ashokrd2013
  • Status changed from new to assigned

Assigning to mark the good-first-bug as "claimed".

#7 @audrasjb
22 months ago

  • Keywords needs-testing removed
  • Milestone changed from Awaiting Review to 5.2

Hi and thanks for the patch!

I tested the patch and it works fine on my side.
The patch still applies cleanly.

Moving to 5.2.

#8 @audrasjb
21 months ago

  • Keywords commit added

This ticket was mentioned in Slack in #core by audrasjb. View the logs.


20 months ago

This ticket was mentioned in Slack in #core by jeffpaul. View the logs.


20 months ago

@audrasjb
20 months ago

BEFORE widget text

@audrasjb
20 months ago

AFTER widget text

This ticket was mentioned in Slack in #core by audrasjb. View the logs.


20 months ago

#12 @audrasjb
20 months ago

  • Keywords has-screenshots added

#13 @pento
20 months ago

  • Keywords needs-design-feedback added; commit removed

Thank you for the GIFs, @audrasjb. This is super helpful for seeing the effect of the change.

I'd like to get a design review on this before we go ahead with it.

This ticket was mentioned in Slack in #design by karmatosed. View the logs.


20 months ago

#15 follow-up: @melchoyce
20 months ago

Chatted about this in today's design triage, and we're generally in favor of this change. We have a couple questions:

  • @pento Do you have any background on why widgets overflow the widget area containers in the first place?
  • @audrasjb Does this patch impact the Customizer as well?

#16 in reply to: ↑ 15 @audrasjb
20 months ago

Replying to melchoyce:

  • @audrasjb Does this patch impact the Customizer as well?

Nope, only the widgets screen.

In my opinion, this is a good candidate for 5.2 since it's pretty self contained.

We can also punt it to the next milestone but that wouldn’t make so much sense to me as this is a bug that can be easily fixed :-)

#17 follow-up: @pento
20 months ago

  • Keywords needs-refresh added; needs-design-feedback removed
  • Milestone changed from 5.2 to 5.3

Thanks for the review, @melchoyce!

I've been doing a little archeology through the codebase, and I have a couple of open questions:

  • Why do we still support wide widgets? @shaunandrews, you were the last person to make a definitive comment on this that I could find.
  • Why are HTML, Text, and RSS widgets wide?

Regarding the patch:

  • We don't actually need to change the height, since it apparently isn't used.
  • The RSS widget is also wide: if we're updating wide widgets, we should do that one, too.

I'm going to punt this issue to 5.3, I'd appreciate if folks could do some more investigation into this.

#18 in reply to: ↑ 17 @westonruter
20 months ago

Replying to pento:

  • Why are HTML, Text, and RSS widgets wide?

I assume the Text widget is wide in order to give more room to edit. The Custom HTML widget is wide because the Text widget was wide, as I recall. I don't know about the RSS widget's reason.

The Text widget has been wide since it was introduced in 2.2 (#4169), as far as I can tell:

The Widgets feature plugin that was merged apparently had wide Text widget, so that is why it persisted. The first feature plugin commit had a widget_text_register() which included the wide widget:

<?php
register_widget_control($name, $i <= $number ? 'widget_text_control' : /* unregister */ '', 460, 350, $i);

https://plugins.trac.wordpress.org/browser/widgets/trunk/widgets.php?rev=5614#L792

#19 @pento
20 months ago

Yeah, it seems like the width of the widget column has changed over the years, both shrinking and growing. I couldn't find any reference when it shrunk it its current size as to why these widgets stayed wide.

Either way, we still support wide widgets, so this is a broader problem: we can make core widgets narrower, but custom widgets will still stuffer the problem originally reported.

@ov1diu
18 months ago

Active Widget Not Fully Visible Ticket ID#42001

#20 @ov1diu
18 months ago

As seen above, the issue doesn't replicate anymore.
Ticket can close.
Thanks!

#21 @printsachen1
18 months ago

  • Resolution set to worksforme
  • Status changed from assigned to closed

I tested also, and it works on 5.3

#22 @audrasjb
17 months ago

  • Resolution worksforme deleted
  • Status changed from closed to reopened

Please do not close tickets that are not committed into core @printsachen1

Reopening.

#23 @audrasjb
17 months ago

  • Owner changed from ashokrd2013 to audrasjb
  • Status changed from reopened to assigned

@audrasjb
17 months ago

Reducing widgets width to 250

@audrasjb
17 months ago

Fixed widget width for all wide widgets

#24 @audrasjb
17 months ago

  • Keywords commit added; needs-refresh removed

Hi there,

In 42001.2.diff:

  • reduce width (and only width) parameter to 250
  • apply that new width to both Text, Custom HTML and RSS Widgets

See animated screenshot above.

This ticket was mentioned in Slack in #core by sergey. View the logs.


16 months ago

#26 @audrasjb
16 months ago

  • Keywords commit removed

Removing commit keyword to see how we can handle custom widgets as well.

#27 @audrasjb
15 months ago

No feedback on this one. Should we commit the current patch so the core widget will be ok or punt it to 5.4 to work on custom widgets as well. I'm all in favor to commit it as it. Any thoughts on that @pento @westonruter ?

#28 @audrasjb
14 months ago

  • Keywords needs-refresh early added; good-first-bug removed
  • Milestone changed from 5.3 to 5.4

The ticket is in my opinion very close to its resolution, but WP 5.3 is already in Beta 2, let's move this ticket to milestone 5.4 early so it could be handled in the next version.

#29 @audrasjb
11 months ago

  • Focuses ui added; javascript removed
  • Keywords commit added; needs-refresh removed
  • Status changed from assigned to accepted

Hi there,

I think we need to fix this issue at least for core widgets. Custom widgets should be addressed in another ticket.
The patch still applies cleanly, so let's tag it for commit.

#30 @audrasjb
11 months ago

  • Keywords needs-dev-note added

This ticket was mentioned in Slack in #core by david.baumwald. View the logs.


10 months ago

#32 follow-up: @SergeyBiryukov
10 months ago

As noted in comment:18, ever since the introduction of widgets in [5294] / #4169, the Text and RSS widgets have been wider than others to provide more space for input. Others generally just have a title input and a few checkboxes and don't require additional width.

The Custom HTML widget was added in [40893] / #40907. Being modeled after the Text widget, it kept the same width.

Some follow-up fixes that touch wide widgets one way or another over the years:

A related IRC discussion from #26117:
https://irclogs.wordpress.org/chanlog.php?channel=wordpress-dev&day=2013-11-19&sort=asc#m732752

My concerns with 42001.2.diff (restricting all widgets to the sidebar width) are:

  • Providing less space for input, making a narrow UI element even narrower, which doesn't seem ideal.
  • It might fix the issue for core, but not for any custom widgets.
  • Removing wide widgets from core and declaring that plugins or themes should no longer be use them seems like a regression, there could be other use cases for this additional width.
  • In general, it feels more like a workaround than really addressing the issue.

Since the main issue here is that an active widget is not fully visible due to overlapping with another widget, I would suggest addressing that.

The widgets on this screen currently have the following z-index hierarchy:

  • Wide widget, open (z-index: 100).
  • Any widget being dragged from the Available Widgets area to a sidebar (z-index: 100).
  • Regular widget, open (no z-index).
  • Any widget, closed (no z-index).

We could utilize the :focus-within CSS pseudo-class (supported by all major browsers) to improve things a bit.

In 42001.4.diff, the hierarchy is adjusted to make sure widget currently being interacted with always stays on top:

  • Any widget being dragged from the Available Widgets area to a sidebar (z-index: 101).
  • Any open widget with a current focus (z-index: 100)
  • Any widget, open (z-index: 99).
  • Any widget, closed (no z-index).

#33 @audrasjb
10 months ago

  • Keywords early commit needs-dev-note removed

Wow, thanks for the detailed comment @SergeyBiryukov !
Currently testing the last proposal.

#34 @audrasjb
10 months ago

  • Keywords commit added

I tested 42001.4.diff and it works like a charm for all modern browsers.

Thanks @SergeyBiryukov for this perfect alternative! Adding commit keyword.

#35 in reply to: ↑ 32 @SergeyBiryukov
10 months ago

Replying to SergeyBiryukov:

As noted in comment:18, ever since the introduction of widgets in [5294] / #4169, the Text and RSS widgets have been wider than others to provide more space for input. Others generally just have a title input and a few checkboxes and don't require additional width.

The Custom HTML widget was added in [40893] / #40907. Being modeled after the Text widget, it kept the same width.

Related: #42081, specifically comment:2:ticket:42081.

#36 @SergeyBiryukov
10 months ago

  • Resolution set to fixed
  • Status changed from accepted to closed

In 47263:

Widgets: Prevent currently active wide widget controls from being partially covered by another widget's controls.

The currently active widget controls should always be displayed on top.

Props audrasjb, fervillz, rinkuyadav999, jaydeep23290, ashokrd2013, melchoyce, pento, westonruter, SergeyBiryukov.
Fixes #42001.

Note: See TracTickets for help on using tickets.