WordPress.org

Make WordPress Core

Opened 7 months ago

Last modified 6 weeks ago

#44836 reviewing defect (bug)

Uploaded plugin installation page: There is an extra <p> tag messing with a link

Reported by: seedsca Owned by: SergeyBiryukov
Milestone: Future Release Priority: normal
Severity: minor Version: 4.9.8
Component: Upload Keywords: good-first-bug has-patch
Focuses: ui, administration Cc:

Description

update.php?action=upload-plugin shows the Activate Plugin button and Return to Plugin Installer link inside the same <p> tag.
This makes the link pop up when the button is active.

If they were both on their own <p> tags or the <p> tag wrapping them was removed, this would not happen.

I could not find where the <p> tag was added or I would have supplied a patch. Any guidance would be appreciated.

Attachments (6)

44836.diff (417 bytes) - added by pratikthink 7 months ago.
Possible fix
4836.2.diff (383 bytes) - added by seedsca 7 months ago.
Activate Plugin button - regular.jpg (8.6 KB) - added by seedsca 7 months ago.
Plugin activate button on regular state. Link looks normal.
Activate Plugin button - active state.jpg (8.8 KB) - added by seedsca 7 months ago.
Plugin activate button on active state. Link moved up by vertical-align: top.
44836.2.diff (375 bytes) - added by seedsca 7 weeks ago.
Correctly renamed the diff file and removed fixed the path inside by removing the /src/ directory. Now using: diff --git a/wp-includes/css/buttons.css b/wp-includes/css/buttons.css
44836.3.diff (366 bytes) - added by seedsca 6 weeks ago.
Seems I forgot to fully remove the /src/ directory from the paths of the diff file. Sorry for all the noice... This is the one :)

Download all attachments as: .zip

Change History (21)

#1 @mukesh27
7 months ago

  • Component changed from General to Upload
  • Keywords good-first-bug needs-screenshots added

Hi @seedsca , welcome to WordPress Trac!

Thanks for the report, Can you please add steps to generate above issue and if possible add screenshot so we can understood it well.

#2 @seedsca
7 months ago

Sure thing!

In the admin page, go to Plugins > Add New > Upload Plugin.
Upload a plugin file and install.
You will then be taken to yoursite.com/wp-admin/update.php?action=upload-plugin

If you click on the Activate Plugin button:

Plugin activate button on regular state. Link looks normal.

You will notice the link to the right jumps up:

Plugin activate button on active state. Link moved up by vertical-align: top.

Hope that helps and again, I'd love to do this myself, but I couldn't fine where the paragraph tag gets added. :)

Last edited 7 months ago by seedsca (previous) (diff)

@pratikthink
7 months ago

Possible fix

#3 @pratikthink
7 months ago

  • Keywords needs-testing added

@seedsca
7 months ago

#4 @seedsca
7 months ago

  • Keywords has-patch added; needs-patch removed

/wp-includes/css/buttons.css line 267 has vertical-align:top set.
This does not seem necessary and it's messing with alignment.

I checked all 28 instances of .button-primary and found no changes at all.

Seems safe.

@seedsca
7 months ago

Plugin activate button on regular state. Link looks normal.

@seedsca
7 months ago

Plugin activate button on active state. Link moved up by vertical-align: top.

#5 @seedsca
7 months ago

  • Keywords needs-screenshots removed

#6 @mukesh27
7 months ago

@seedsca 4836.2.diff is working fine for me. Let's wait for core developer response on above solution.

#7 @ianbelanger
7 months ago

  • Keywords needs-testing removed

Hi @seedsca and @pratikthink,

Thanks for your patches. I tested 4836.2.diff, which should probably be named 44836.2.diff, and it seems to work great as a solution to the "jumping" link issue. I didn't see any adverse changes to any other buttons that use the .button-primary class.

This looks good to merge in core.

#8 @SergeyBiryukov
7 months ago

  • Milestone changed from Awaiting Review to 5.0
  • Owner set to SergeyBiryukov
  • Status changed from new to reviewing

#9 follow-up: @seedsca
7 months ago

@ianbelanger and @SergeyBiryukov Thanks!
Should I change the diff file's name? Or are we all set?
Happy to help the community that's helped me so much :)

#10 @ianbelanger
7 months ago

I don't think that there's any need to change the name of the diff @seedsca, unless Sergey says differently.

#11 in reply to: ↑ 9 @SergeyBiryukov
7 months ago

Replying to seedsca:

Should I change the diff file's name? Or are we all set?

Thanks for the ticket and the patch :) It's generally helpful if the name of the diff is the same as the ticket number, but ultimately it doesn't matter that much, the patch is fine as is.

#12 @pento
5 months ago

  • Milestone changed from 5.0 to 5.1

This ticket was mentioned in Slack in #core-media by joemcgill. View the logs.


2 months ago

#14 @joemcgill
2 months ago

  • Milestone changed from 5.1 to Future Release

Punting this to a future release. Unless @SergeyBiryukov is still planning to review this, we should remove the assignment so someone else will pick it up.

@seedsca
7 weeks ago

Correctly renamed the diff file and removed fixed the path inside by removing the /src/ directory. Now using: diff --git a/wp-includes/css/buttons.css b/wp-includes/css/buttons.css

@seedsca
6 weeks ago

Seems I forgot to fully remove the /src/ directory from the paths of the diff file. Sorry for all the noice... This is the one :)

#15 @seedsca
6 weeks ago

@SergeyBiryukov anything else to do with this little patch? Seems simple enough.

Note: See TracTickets for help on using tickets.