Bartik theme doesn't allow to quick edit image fields

Proposed solution is not good. If we remove float from the image field we remove the intended functionality from theme. Attached is my solution to the problem.

Also moved this to 8.4.x-dev because it is still present in latest version.

@oakulm's patch makes more sense.

tested for the patch by @oakulm. Works as intended.

+1

The #7 patch breaks the clear on comments.

Attached is better solution for this.

Z-index value is based on the fact that Toolbar has z-index: 501 so we are trying to keep images below Toolbar.

Also I have added clear for comments as they are breaking quick edit if no body text is present.

I tested the patch -13 to be a working one. As Bartik doesn't have comments wrapper and node links templates this seems to be a reasonable solution.

1. +++ b/core/themes/bartik/css/components/field.css
   @@ -46,10 +46,14 @@
   +    z-index: 480;
   ...
   +    z-index: 480;
   

   Let's document the rationale behind using this particular z-index.

2. +++ b/core/themes/bartik/css/components/node.css
   @@ -54,6 +54,7 @@
    .node__links {
      text-align: right; /* LTR */
      font-size: 0.93em;
   +  clear: both;
    }
   

   Please provide a screenshot showing that this change does not break node links.

I triaged this and found that this is still an issue and the latest patch still applies.

darrindeal, please provide screen for #18?

@darrindeal I am going over the issues analyzed at the Baltimore triage in order to assign credit. I would like to give you credit for the work done, but I can't tell whether or not all steps of the triage were completed by reading the comments. We like to see documented steps (even if brief). Here are some made-up examples of documented triage steps:

• I tested the steps to reproduce and they did (or did not) work (so I am tagging it "Needs issue summary update").
• I searched for duplicate issues but could not find any.
• I checked the issue summary and it is accurate and up-to-date.
• Etc...

Thank you!

Please review as this should be fine.

hi @oakulm

Your patch applies cleanly and quick edit works even if the image is there. Please see the screenshot

The feedback from #18 haven't been addressed yet.

This is first time me writing comments on Core CSS. Is the formatting ok?

Also there is a screen cap for node links.

Sorry to keep changing this but after some more testing z-index of 400 something was too much as images jumped over quick edit forms. Now z-index is so that it should be below quickedit forms and Toolbar.

Does not require re-roll - current patch (#31) still applies and passes tests.

Thanks for the patch. I tested this with the article content type. When I first tried it, the image was above the body field and I could not get the quick edit to show up for the image. When I moved the image below the body field, I was able to get the quick edit to show up for the image field. I tried with 2 different image sizes and it didn't make a difference. See screenshots below. I tested on Chrome 63. I didn't see any issue with the node links. As for the CSS comments, I normally see them at the end and inline but these are long comments. Also, although I do see some core comments start with "We", I would avoid it.

+++ b/core/themes/bartik/css/components/field.css
@@ -47,10 +47,16 @@
+    /* We are keeping images below Toolbar and below Quickedit form. */
+    z-index: 99;

Perhaps simplifying to something like:

z-index: 99; /* Keep below Toolbar and Quickedit */

Thanks for the review! And here you go with better comments.

Thanks, @oakulm. An interdiff would be handy. It looks like you only changed the comments. If so, there is still the issue that the image is not editable when it's positioned above the body field so I'm marking this back to needs work.

Yes you are correct. I missed your comment about the above the body field. I will take a look

Here you go. So I had to raise the z-index back up again to prevent body from wrapping over the image. The problem is with quickedit form that has fixed z-index and it has to be modified in Bartik to fix this issue.

Patch applies cleanly, and fixes the issue when the image is above the body field, see attachment.

Setting this back to needs review and triggerering a retest. I think something was wrong with the testbot because the test passes for me locally.

Since the tests are all passing now, I'm changing the status of this issue back to RTBC.

I don't seem to be able to reproduce the bug. Could someone update the issue summary to include the specific steps required for reproducing this?

+++ b/core/themes/bartik/css/components/comments.css
@@ -10,6 +10,9 @@
+.comment-wrapper {
+  clear: both;
+}

+++ b/core/themes/bartik/css/components/node.css
@@ -54,6 +54,7 @@
+  clear: both;

We should open a separate issue for this since this seems unrelated to the bug reported here.

I think this is still a Novice issue. Per @laurii's comments, the issue needs clear steps to reproduce in Drupal 9.1.x, and the issue summary updated.

On a fresh install of Drupal 9.x with the standard profile, you can recreate the issue as follows:

1. Create new Article content with text in the body field and in image
2. When viewing the article, enable Quick edit
3. Mouse over the image and notice that quickedit chooses the body field over the image field

While the patch does remedy this problem for Bartik, I believe the solution should be in the quickedit module to properly handle inline field elements within another field. This issue is a duplicate of [#2697500].

Updated the issue summary with the steps to reproduce from #51. Still leaving the tag in case more clarification is needed.

Hi, i'm a bit new but try to help, i've read issue and removed "clear: both;" property because @laurii suggested it should be another issue in my attached patch.
Should we finish this issue first and then open a global one to handle inline field elements within another field in quickedit module as @evilehk suggested ?

Tested the patch manually
We are able to use quick edit for editing images added via image field and also images in body field

If more review is needed then that can be done otherwise this can be moved to RTBC

I was testing again without a patch, both with separate fields (body+image) and with an image in the body field, and noticed something odd. When I first did the quick edit, then the quick edit controls positioning was off. But, if I exited quick edit and then opened it again, the positioning seemed ok. Example:

First time clicking quick edit

Second time clicking quick edit

This was after clicking the X to get out of it. This was reproducible if I reloaded the page and started again.

I also tested the patch from #53 and unfortunately had issues.

Text and image both in body field

Quick edit doesn't let me get to the text (which is above the image). Closing with the X and trying again didn't help.

Separated image and body fields

Note I'm testing on simplytest.me and there is an issue showing images in image fields so that is compounding things. But, I still think there is something weird going on because I can't see the body text to be able to edit that. Though, it's possible that it's just because simplytest.me isn't showing the image due to a bug in their code: #3162126: Image styles either aren't generated or can't be downloaded.

Moving back to needs work after more testing.

1) Tested with separate image field and body text and, although I could replace the image, the quickedit controls are hidden for both the image and the body text. See screenshots.

2) Tested with image within the body text and I'm able to change the image but not able to edit the body text above the image. See screenshots.

3) Tested use case 1 with Claro and Seven just to make sure they work ok there and they do.

4) Tested use case 2 with Claro and Seven just to make sure they work ok there and they also do not work. All themes had a consistent behavior of hiding the text above the image. See screenshots.

5) Since the issue summary is focused on image fields then I'll check if 2/4 are covered by a different issue.

Bartik with separate image field - Initial image

Bartik with separate image field - After replacing image

Bartik with image in body field - can't edit text above image

Claro with image in body field - can't edit text above image

Seven with image in body field - can't edit text above image

I found the issue related to the problem in #56 when image is in the body field. But, it's a general problem and isn't related to the fact there is an image in the body field.

#2350535: Quick Edit toolbar does not float up above content on first invocation, it covers the text in the Body field

I performed Novice Triage on this issue. I am leaving the Novice tag on this issue only because I think that the issue summary needs to be updated. It would be great if someone could update the Steps to reproduce section with some of the excellent manual testing done by @Kristen Pol above. Also add what the proposed resolution is to address the bug in the patch.

After updating the issue summary, if we think that there is a clear path forward for addressing the one issue from the review in #56, then leave the Novice tag. Otherwise remove the Novice tag.

Hi all,

As per the issue i create patch regarding this issue please review and gave feedback.

Thanks!

Changed the order for CSS,
The patch was small so didn't make interdiff file

Hi all,

i am adding new patch with interdiff file and also got my mistake what i done.

Please review and give feedback.

Thanks!

Applied patch #62 and it works fine.Now the image field allows quickedit. Adding screenshots bellow.

Before patch:

After patch:

RTBC +1

Patch in #62 is very close, but I found a situation where the edit popup gets hidden behind the image like so:

Attached patch fixes this issue by using a slightly "lower" z-index.

@markandrewsutton i did not found any situation where the popup gets overlapped by the image. Added a screenshot for ref.

Applied patch #62 and it works fine. Now the image field allows quick edit
Adding screenshot for ref.
RTBC +1

This is failing CSS linting, see https://www.drupal.org/pift-ci-job/1973192

Removed the whitespace. Kindly review my patch.

Applied patch #68 and it works fine. Now the image field allows quick edit
Adding screenshot for ref and video.
RTBC +1

Patch #68 applied cleanly and it is working well.
Before patch:

after patch:

@Wim Leers asked for a comment explaining why the z-index is the particular value. This was present before #60 but then disappeared. It's important to add a comment documenting z-index values to have us maintain them. Also I think this is testable via WebDriverTestBase - and given the fragility of z-indexes I think it is worth doing.

Applied patch #62 successfully and looks good for me
for ref sharing screenshot....

Moving to contrib since this is specific to Bartik, which was removed from core in Drupal 10.