adobe / brackets

An open source code editor for the web, written in JavaScript, HTML and CSS.
http://brackets.io
MIT License
33.26k stars 7.63k forks source link

[CEF 2171] Collapsed tree nodes have no text #9965

Closed JeffryBooher closed 9 years ago

JeffryBooher commented 9 years ago

Open the brackets project and collapse all top-level nodes all disclosure triangles.

Expanding the tree node shows it but clicking other node will hide it again. Feels random.

Also, the context-selected styles are wrong.

peterflynn commented 9 years ago

Crap, this appears to be a browser repaint bug. The CSS seems fine as-is, and kicking it to update by adding and removing a color: red on any given node (i.e. a no-op style change) causes that one node to start displaying correctly.

This immediately makes this CEF update seem much more risky :-(

peterflynn commented 9 years ago

On a hunch, I tried removing the "quiet scrollbars" stuff, but that didn't fix it.

However, if I pull #project-files-container out of #sidebar and dump it as a following sibling, the issue appears to go away. Seems worth ripping out pieces of styles & layout stuff from #sidebar to try to identify what's acting as a trigger.

peterflynn commented 9 years ago

Ugh, not looking good as far as an easy smoking gun. I reduced it a bit to this:

    <div class="main-view">
        <div id="sidebar" class="sidebar panel">
            <div class="working-set-splitview-btn btn-alt-quiet"></div>
            <div class="working-set-option-btn btn-alt-quiet"></div>

            <div id="working-set-list-container">

            </div>
            <div id="project-files-header">
                <span id="project-title" class="title"></span>
            </div>
        </div>
        <div class="fake-sidebar">
            <div id="project-files-container">
                <!-- This will contain a dynamically generated <ul> hierarchy at runtime -->
            </div>
        </div>

and

.fake-sidebar {
        width: 200px;
}

Removing the width there will make the bug go away. Uhhh...?

Notably, this removes the flexbox layout that is on a normal .sidebar. Also, it doesn't matter what the width value is -- I can make it 1000px and the labels are still all missing.

peterflynn commented 9 years ago

Interestingly, Chrome stable just hit 39.0.2171.65 and this new CEF is also 39.0.2171.65 -- so one thing we can do is run Brackets in the browser and see if the same bug repros there. I'll start dusting off my in-browser branch so we have it available for that...

dangoor commented 9 years ago

Scary. If this does repro in Chrome stable, then we'd be pretty sure to get a fix.

JeffryBooher commented 9 years ago

There's no screen grab, unfortunately, but #8133 is what we were seeing with 1916

peterflynn commented 9 years ago

Sadly, I cannot get this to repro in browser yet. If anyone else would like to give it a try, I've pushed up pflynn/in-browser-dummy-subfolders -- a sub-branch of the in-browser version that has a more complex file tree, and a slight async delay on IO operations to approximate the timing seen in real Brackets.

peterflynn commented 9 years ago

Aha! I can repro this with the simple in-browser version in the CEF 2171 brackets-shell. In theory that code couldn't rely on any Brackets-only shell APIs, so it should work as a repo case in vanilla cefclient too (but still not Chrome itself) -- perfect testcase for Marshall.

@JeffryBooher How hard is it to make a vanilla cefclient build? Is that in our grunt script or anything? If not, would you mind posting a copy somewhere so I could play with it?

JeffryBooher commented 9 years ago

@peterflynn you will find it in your deps/cef folder under brackets-shell. Open cefclient.vcxproj in Visual Studio and build/run it. That will get you a basic web browser with an address bar that you should be able to enter a file:// URL to your test page

peterflynn commented 9 years ago

@JeffryBooher I get a linker error: "cannot open input file '...\deps\cef\out\Debug\lib\libcef_dll_wrapper.lib'" (not sure why it's looking there since I had selected the Release configuration, but \lib is empty in both Debug & Release anyway...)

peterflynn commented 9 years ago

I found a libcef_dll_wrapper.sln in the same folder, but if I open that and make a Debug build (still no idea why it's not looking in the Release folder), then I get more link errors in the cefclient project. Most of them are "mismatch detected for '_ITERATOR_DEBUG_LEVEL': value '2' doesn't match value '0' in cefclient.obj." Any ideas?

peterflynn commented 9 years ago

Aha, looks like you just can't make Release builds of cefclient. I got a Debug build working though -- and hurrah!, the labels are consistently broken there too.

Hopefully this is good enough to package into a testcase now, since there's a clear rendering diff between CEF and Chromium.

JeffryBooher commented 9 years ago

@peterflynn my bad, you were supposed to open cefclient2010.sln in Visual Studio which will build all of the dependencies.

RaymondLim commented 9 years ago

I found the culprit. If I comment out the following CSS properties that can trigger the GPU mode, then the issue is fixed.

/** Overriding jsTreeTheme.less
*/
.jstree-brackets .jstree-no-dots .jstree-open > ins {
    background-position: 7px -8px;
/*    -webkit-transform: translateZ(0) rotate(90deg);
    -webkit-transition: -webkit-transform 190ms cubic-bezier(.01, .91, 0, .99);
    -webkit-filter: drop-shadow(1px 0 1px @bc-shadow);*/

/*    .dark & {
        -webkit-filter: drop-shadow(1px 0 1px @dark-bc-shadow);
    }*/
}

.jstree-brackets .jstree-no-dots .jstree-closed > ins {
    background-position: 7px -8px;
/*    -webkit-transform: translateZ(0);  Need this to make sure that the svg isn't blurry on retina. 
    -webkit-transition: -webkit-transform 90ms cubic-bezier(.01, .91, 0, .99);
    -webkit-filter: drop-shadow(1px 0 1px @bc-shadow);*/

/*    .dark & {
        -webkit-filter: drop-shadow(1px 0 1px @dark-bc-shadow);
    }*/
}

So this is definitely related to #9978 which is caused by translateZ, but we also need to report that -webkit-filter property can trigger the GPU mode and causes text not to render.

RaymondLim commented 9 years ago

Actually, the real culprit is not -webkit-filter as mentioned in my previous comment. I still see the very first item (either file/folder) of the tree not rendered with the above commented code. If I instead comment out the following lines in jsTreeTheme.less file, then the entire tree is rendered.

.jstree-brackets li > a {
/*    @jstree-icon-text-overlap: 2px;
    padding-left: (@jstree-sprite-size - @jstree-icon-backindent - @jstree-icon-text-overlap + 10000px);
    margin-left: -10000px;*/
    display: block;
}

It seems like our trick to render the tree offscreen (in negative coordinate) and then bring it back with margin-left and padding-left properties are ignored by latest Chromium.

peterflynn commented 9 years ago

In my in-browser/cefclient testcase, removing -webkit-filter fixes the persistent bug, but the labels still briefly disappear whenever the tree is updated. Removing the huge padding-left and margin-left from the rule above fully fixes the bug.

I searched for other places where we use large values in CSS, and luckily nothing else looks too similar to this. (.dummy-text uses a very large negative top, but it's meant to stay offscreen -- not be pushed back onscreen by a similarly large positive value).

peterflynn commented 9 years ago

I filed https://code.google.com/p/chromiumembedded/issues/detail?id=1452 on the CEF bug, but it does appear that we can safely work around it in this case by removing the margin/padding hack, which is no longer needed with our new React-based tree widget.

peterflynn commented 9 years ago

Confirmed fixed in Brackets. Although the underlying CEF/Chromium issue is still present (see bug linked above), I don't think this needs to stay open tracking a fix since we don't have reason to believe anything else in Brackets will trigger that bug. Closing.